Sorry, meant to make this a draft

@sammccall I think you can merge this for me now (and also https://reviews.llvm.org/D128202)

• Remove obsolete comment
• Return empty vector when type isNull()
• Add comment about why we don't deduplicate results
• Use out-parameter in unwrapFindType to make structure more consistent

In D128826#3619956, @tom-anders wrote:

So here's what I thought: Say you have a variable that's a smart pointer and trigger textDocument/typeDefinition on it and there's only a single result that takes you straight to the type. In this case, you might not even know that the type is actually wrapped inside a smart pointer and might make wrong assumptions about the code. Of course you could've done a Hover beforehand, but in the case where there are 2 results, you don't need that extra step.

Yeah. There's no guarantee the type in question is well-represented by a decl, but this is a bit confusing.
I think it might still be worth it for specific types like unique_ptr<Foo>, but for vector<Foo> it's too surprising even if you want Foo 99% of the time.

Right, maybe it would make sense to go straight to T for declarations, but include smart_ptr when it's used in an expression? But that's something for a follow-up patch.

Hmm, is the distinction that the type is written in the declaration (not auto)? I'm not sure it's worth specializing behavior for that scenario too much, when it's not too hard to go-to-definition on the specific token you want.
The chance of guessing right has to be weighed against keeping the behavior predictable and comprehensible...

Yes that's what I meant, but I agree with you here.

In D128826#3619339, @tom-anders wrote:

I'm using this with neovim's builtin LSP, where when there are multiple results it takes me to the first one, and also opens the quickfix window that shows all results. But I can see how the current implementation could be annoying with other editors.

(Interesting, I'm using the same and find it annoying: it focuses the quickfix window and that's disruptive enough that I never noticed the cursor also moved. But https://github.com/neovim/neovim/pull/14878 lists some workarounds)

(Ah, I also forgot that I'm not actually using quickfix, but rather https://github.com/folke/trouble.nvim, which also gets focused but has a preview feature)

In D128826#3619167, @sammccall wrote:

Thanks! I just have a question if this behavior should be even "stronger":

In most editors, if you return one result you go straight there, if you return two results you get a menu.
A menu is significantly worse than going straight to the right result - the UI is clunkier and the choice interrupts your workflow.

So my question is, do you think that we should *only* return the pointee in these cases?

(It's possible to change this later, but I'd like to take a reasonable guess as this is forcing an API change from Type -> vector<Type>)
cc @kadircet

Check for FinalAttr instead of using isEffectivelyFinal()

I’d say commit as is and have a look at the SmallVector -> unit32_t thing together with the std::map issue afterwards

Yeah, I‘d need someone to commit this for me.

• Use llvm::SmallVector, add more FIXME, remove old commented out test

@sammccall @nridge Thanks for the positive feedback, let me know if I missed any of your suggestions!

• clang-format once again

• Remove accidentally added empty line

• Apply suggested renaming and fix nits
• Use a map for extra modifiers instead of abusing conflict resolution
• Use ArrayRef instead of callback
• Add FIXME for handling CXXOperatorCallExpr
• Use getLocation() instead of getBeginLoc()
• Add FIXME for unwrapping operators
• Iterate over FunctionProtoType instead of FunctionDecl
• Adjust highlighting condition and revise test cases