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 = {});
----------------
ilya-biryukov wrote:
> 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.
Note that rename and other features using `targetDecl` now would actually be
totally fine if they switched to the new function.
They actually don't care, but the new function has a simpler interface.
It was never intended to be a replacement. `targetDecl` does more stuff.
However, this one can have a simpler interface. I prefer to have both and use
the one with a simpler interface whenever possible.
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
