ilya-biryukov 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 = {});
----------------
sammccall wrote:
> ilya-biryukov wrote:
> > sammccall wrote:
> > > 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.
> > >
> > Here's rough description of the concept:
> > - pick the most specific Decl, whenever possible. That means instantiations
> > for templates instead of patterns and never look through typedefs (Mask has
> > to be removed, this needs a separate refactoring).
> > - I described behaviour on particular examples you mentioned.
> >
> > It's useful because it's a simple way to get the most specific thing
> > written in the source code (even for implicit nodes, we choose to not do
> > complicated things like looking through typedefs).
> >
> > I suggest to call it `getReferencedDecls`.
> >
> > > 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.
> >
> > We can get the underlying type in that particular case inside hover, it's
> > just a few lines of code. I don't think there are other cases where we
> > loose information.
> >
> >
> > > 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?
> > Rename uses `findExplicitReferences`, which uses `explicitReferenceTargets`
> > internally. XRefs, Rename, Code actions and any refactorings (if we ever do
> > any of those) will benefit from it, but mostly indirectly by using
> > `findExplicitReferences`. It also provides information about source
> > locations of the identifiers that name things, which is crucial for many of
> > those features.
> >
> >
> > > 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.
> >
> > They would sometimes need to go from template instantiation to template
> > declaration or template pattern and (very rarely) get from typedef to its
> > underlying type. Both are very simple operations.
> > I described behaviour on particular examples you mentioned.
> Those examples weren't cases where I was questioning the behavior, they were
> examples of where the behavior wasn't motivated by the description.
>
> The questions I'm referring to are:
> > - 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?
>
> I think the latter is answered but not the former.
>
> > We can get the underlying type in that particular case inside hover, it's
> > just a few lines of code. I don't think there are other cases where we
> > loose information.
> There are several cases - at least alias instantiations and using decls of
> overloads. Note that the reported alias decl is the UsingDecl, not the
> UsingShadowDecl. (Changing that case is possible, but would make more work
> for all the other callers).
>
> > I suggest to call it getReferencedDecls.
> This doesn't describe the distinction from targetDecl().
>
> > Rename uses findExplicitReferences, which uses explicitReferenceTargets
> > internally. XRefs, Rename, Code actions and any refactorings (if we ever do
> > any of those) will benefit from it, but mostly indirectly by using
> > findExplicitReferences.
>
> findExplicitReferences is certainly useful and has a clear API, but that
> doens't require `explicitReferenceTargets` to be public, which is what I'm
> asking here.
>
> Rename uses `targetDecl` together with `TemplatePattern`, which is the
> callsite I was referring to there.
>
> If the answer is that 2/7 current callsites will prefer
> `explicitReferenceTargets` and 5/7 need `targetDecl`, that doesn't seem like
> most code, nor does it seem self-evident that code actions added in the
> future will fall into the first bucket.
> how often is "most specific" the thing we want, vs "decl that's in source
> code" or "everything for completeness"
As mentioned before, I believe the following features would benefit from this:
rename, cross-references, code actions and other refactorings.
> Note that the reported alias decl is the UsingDecl, not the UsingShadowDecl.
> (Changing that case is possible, but would make more work for all the other
> callers).
What are the callers that would need more work for `UsingShadowDecl`? I lack
context to understand this example.
This function is useful because it's simpler to use in some cases than
`targetDecl` (if we get rid of `Mask`).
Sure, you **could** write all code using `targetDecl`, it's also ok to add a
few helpers like this to simplify some common cases from my POV.
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
