sammccall added inline comments.
================
Comment at: clang-tools-extra/clangd/FindTarget.h:194
+llvm::SmallVector<const NamedDecl *, 1>
+explicitReferenceTargets(ast_type_traits::DynTypedNode N,
+ DeclRelationSet Mask = {});
----------------
ilya-biryukov wrote:
> sammccall wrote:
> > ilya-biryukov wrote:
> > > sammccall wrote:
> > > > ilya-biryukov wrote:
> > > > > sammccall wrote:
> > > > > > ilya-biryukov wrote:
> > > > > > > No need to fix this.
> > > > > > >
> > > > > > > The name could probably be better, but we can fix this later.
> > > > > > > Don't have any good ideas.
> > > > > > I think as a public API this should be clearer on how/when to use
> > > > > > and its relation to other things (planning to send a patch, but
> > > > > > wanted to discuss a bit here first).
> > > > > >
> > > > > > - This is essentially a filter of allTargetDecls (as is
> > > > > > targetDecl), but the relationship between the two isn't clear. They
> > > > > > should have closely related names (variant) or maybe better be more
> > > > > > orthogonal and composed at the callsite.
> > > > > > - The most distinctive word in the name is `explicit`, but this
> > > > > > function doesn't actually have anything to do with explicit vs
> > > > > > non-explicit references (just history?)
> > > > > > - It's not clear to me whether we actually want to encapsulate/hide
> > > > > > the preference for instantiations over templates here, or whether
> > > > > > it should be expressed at the callsite because the policy is
> > > > > > decided feature-by-feature. `chooseBest(...)` vs
> > > > > > `prefer(TemplateInstantiation, TemplatePattern, ...)`. The latter
> > > > > > seems safer to me, based on experience with targetDecl so far.
> > > > > >
> > > > > > A couple of ideas:
> > > > > > - caller invokes allTargetDecls() and then this is a helper
> > > > > > function `prefer(DeclRelation, DeclRelation, Results)` that mutates
> > > > > > Results
> > > > > > - targetDecl() becomes the swiss-army filter and accepts a list of
> > > > > > "prefer-X-over-Y", which it applies before returning the results
> > > > > > with flags dropped
> > > > > I don't like the idea of `targetDecl` becoming the swiss-army knife,
> > > > > it's complicated to use as is.
> > > > > The contract of `explicitReferenceTargets` is to expose only the
> > > > > decls written in the source code and nothing else, it's much simpler
> > > > > to explain and to use.
> > > > >
> > > > > It's useful for many things, essentially for anything that needs to
> > > > > answer the question of "what does this name in the source code refers
> > > > > to".
> > > > >
> > > > > The name could be clearer, the comments might need to be improved.
> > > > > The only crucial improvement that I'd attempt is getting rid of the
> > > > > `Mask` parameter, I think there was just one or two cases where it
> > > > > was useful.
> > > > >
> > > > >
> > > > > I don't like the idea of targetDecl becoming the swiss-army knife
> > > > Fair enough. I'll poke more in the direction of making it easier to
> > > > compose allTargetDecls then.
> > > >
> > > > > The contract of explicitReferenceTargets is to expose only the decls
> > > > > written in the source code and nothing else, it's much simpler to
> > > > > explain and to use.
> > > > So I agree that the interface of targetDecl() is complicated, but I
> > > > don't think this one is simpler to explain or use - or at least it
> > > > hasn't been well-explained so far.
> > > >
> > > > For example,
> > > > - "to expose only the decls written in the source code" -
> > > > `vector<int>` isn't a decl written in the source code.
> > > > - "find declarations explicitly referenced in the source code" -
> > > > `return [[{foo}]];` - the class/constructor isn't explicitly
> > > > referenced, but this function returns it.
> > > > - "what does this name in the source code refers to" - a template name
> > > > refers to the primary template, the (possible) partial specialization,
> > > > and specialization all at once. Features like find refs/highlight show
> > > > the equivalence class of names that refer to the same thing, but they
> > > > don't prefer the specialization for that.
> > > > - none of these explanations explains why the function is opinionated
> > > > about template vs expansion but not about alias vs underlying.
> > > >
> > > > > No need to fix this.
> > > > > The name could probably be better, but we can fix this later. Don't
> > > > > have any good ideas.
> > > >
> > > > I think it's fine (and good!) that we this got added to unblock the
> > > > hover work and to understand more use cases for this API. But I do
> > > > think it's time to fix it now, and I'm not convinced a better name will
> > > > do it (I can't think of a good name to describe the current
> > > > functionality, either). Let me put a patch together and take a look?
> > > What's the proposed fix?
> > >
> > > I might miss some context with the examples you provided, but here's my
> > > view on how each of those should be handled:
> > > - `vector<int>`. This function **can** return decls, not written in the
> > > source code (e.g. template instantiations). References to those decls
> > > should be written in the source code explicitly, not the decls themselves.
> > > - `return {foo}`. I agree this one is a corner case that we should
> > > describe. It still provides sensible results that could be used by hover,
> > > etc.
> > > - "what does this name in the source code refers to". That's exactly what
> > > this function is about. We want to pick the most specific thing possible
> > > (i.e. the instantiation). Client code can go from instantiation to
> > > template pattern, it can't go the other way. There are exceptions where
> > > returning instantiation is not possible - instantiations of template type
> > > aliases, dependent template instantiations. We choose template pattern
> > > there, but we don't really loose any information (if we choose to return
> > > Decl, there's nothing else we can return).
> > > - "none of these explanations explains why the function is opinionated
> > > about template vs expansion but not about alias vs underlying." That I
> > > agree with, I think it's possible to make it opinionated there and remove
> > > the `Mask` parameter. I'm happy to take a look, but don't want to clash
> > > with you if you're doing the same thing. Would you want me to give it a
> > > try?
> > >
> > > What's the proposed fix?
> > (FWIW I wrote this post upside-down, because I don't/didn't have the ideas
> > worked out and this discussion has shaped my thinking a lot)
> >
> > My proposal would be to evolve both `targetDecl()` and
> > `explicitReferenceTargets` into composable functions that help select the
> > desired results of `allTargetDecls()`. Policy would be encoded at
> > callsites, so functions would be quite specific (e.g.
> > `preferTemplateInstantations`), or data-driven `filter(DeclRelationSet)`.
> > Some reasoning below - basically I'm not convinced that the "do what I
> > mean" behavior is well-defined here.
> >
> > > I agree this one is a corner case that we should describe. It still
> > > provides sensible results that could be used by hover, etc.
> >
> > Yeah, it's the right behaviour. My point is that I don't think the policy
> > being implemented here is "explicitness" - another example from a recent
> > patch, CTAD `^vector x{1,2,3}` we want the instantiation.
> >
> > Maybe it's "specificness", but this raises questions:
> > - how often is "most specific" the thing we want, vs "decl that's in
> > source code" or "everything for completeness"
> > - does the concept of 'specificness' generalize to alias vs underlying?
> >
> > My impression is that it's *sometimes* what we want, but not overwhelmingly
> > often (e.g. not go-to-def or documentHighlights) and that it doesn't
> > generalize well[1]. If that's the case, I think this should be apolicy
> > expressed at callsites in features (`preferTemplateInstantiations` or
> > something).
> >
> > It would be nice if there's some global concept of "preferredness", but
> > looking at the needs of different features, I really don't see one - that's
> > why targetDecl is so complicated.
> >
> > [1] Example of it not generalizing well - for hover we want instantiations
> > over patterns, but after discussing with Kadir I think we want *both* the
> > alias and the underlying so we can show the docs/association from the alias
> > and the structured info from the underlying.
> - `vector x{1,2,3}` should definitely return an instantiation, that aligns
> with the contract of the function.
> I know explicit is a bad name for this concept, it's more intricate.
>
> I would argue we want the most specific one for hover and if we want to look
> into the underlying decls hover code is simple if it does so explicitly,
> rather than passing custom filters to `allTargetDecls`.
>
> My impression is that evolving this into custom filter for `allTargetDecls`
> makes the callsites more complicated. Having a single "swiss-knife"-style
> function is good for sharing implementation, but the client code is more
> complicated and error-prone with it.
>
> I would strongly suggest to keep `explicitReferenceTargets`, most code is
> simpler with it. Most "filtering" and "getting underlying decls" can be done
> in the client code and the client code is more straightforward with it.
> I know explicit is a bad name for this concept, it's more intricate.
So at this point, I don't understand what the concept is, neither the name nor
the doc comment describe it well. If you want to keep it, can you describe what
the concept is at a high level and how it should interact with aliases, and
suggest a name that is specific enough that it at least hints at the true
distinction vs generic allTargetDecls()?
> rather than passing custom filters to allTargetDecls
allTargetDecls doesn't accept filters, and I don't suggest changing that. Are
you referring to targetDecls? If so I think we're agreed on that point.
> I would argue we want the most specific one for hover and if we want to look
> into the underlying decls hover code is simple if it does so explicitly
I guess you're saying the alias is more specific here? Unfortunately the alias
doesn't preserve the underlying decl in general. We need both decls - one
preserving the alias and the other preserving the type arg/overload.
> I would strongly suggest to keep explicitReferenceTargets, most code is
> simpler with it. Most "filtering" and "getting underlying decls" can be done
> in the client code and the client code is more straightforward with it.
This is what I'm not convinced of. Currently there are 2 users of
explicitReferenceTargets, the original internal user and now hover. Which of
the other targetDecl callsites (Rename and 5 in XRefs) can use
explicitReferenceTargets? How much "getting underlying decls" would they become
responsible for? targetDecl() was in part an effort to unify this unwrapping
because I found it too difficult to maintain the mechanical bits when they were
embedded in each feature.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D71596/new/
https://reviews.llvm.org/D71596
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
