[PATCH] D71596: [clangd] Improve documentation for auto and implicit specs

Fri Jan 3 09:09:15 PST 2020

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