Analysis server has broken navigation link from enum .values

[stereotype441]

Repro: using an editor with Dart analysis server integration (like IntelliJ), find a reference to the .values getter of an enum class (for example, HintActionKind.values on line 14 of pkg/nnbd_migration/lib/src/hint_action.dart). Click on the identifier values and ask the editor to navigate to the definition.

Expected result: the user should be navigated to the declaration of the enum.
Observed result: the user is directed to the top of the file containing the enum.

[stereotype441]

It looks like there is a similar problem for .index.

[jcollins-g]

possibly related: dart-lang/dartdoc#2660

[stereotype441]

@jcollins-g Interesting! I discovered this issue in the process of trying to diagnose a similar issue with the migration tool. Perhaps we can find a way to have a single bug fix that is shared by all three tools.

[bwilkerson]

For the navigation piece, the problem is that each identifier is resolved to a synthetic element, which by definition has no source offset (so we return -1 from offset). I'm not thinking of a good way to handle this generally, so I suspect this will need to be handled on a case-by-case basis.

[jcollins-g]

For the navigation piece, the problem is that each identifier is resolved to a synthetic element, which by definition has no source offset (so we return -1 from offset). I'm not thinking of a good way to handle this generally, so I suspect this will need to be handled on a case-by-case basis.

This is why I didn't file an analyzer issue originally; since it was a synthetic element causing the problem I simply assumed I'd have to special case it similarly to other synthetics. I don't know how to handle it generally -- I know what would work for me, which is referring to the location of the element that implied the synthetic element -- but I don't know what the consequences are outside of dartdoc to making such changes.

[bwilkerson]

That's an interesting possibility. I'm not sure whether it would make the API easier or harder to use. Some tools would still need to be aware of the fact that the element is synthetic and my concern is that it would be much easier to write invalid code that doesn't crash and hence gives no indication that it's producing the wrong results. Having a negative offset might actually allow bugs to be caught sooner.

If we don't change the current semantics of offset, we could consider defining a new getter (navigationOffset?) that would have a semantics that both the navigation producer and dartdoc could use.

[stereotype441]

I like the idea of having a navigationOffset getter. That would avoid the need to address bugs like this in multiple places (currently we have had to fix this issue in at least 3 places: the analysis server, dartdoc, and the migration tool).

[scheglov]

One issue with navigationOffset is that it provides you only with partial information. You get the offset, but you don't know why. For an implicit getter the reason is that you should navigate to the corresponding property, and here to an enum constant.

What if we return the navigationElement instead, which is guaranteed to have a non-negative offset (for unnamed libraries the navigation element is the defining unit, and the offset is zero)? Unfortunately I don't how to express in types that the offset is non-negative, or positive. So, it would be just a convention.

[bwilkerson]

That's even better. It's more general than my original proposal, though, so perhaps a more general name such as nonSyntheticElement would be in order. (There are probably plenty of places in analyzer, server and linter that do the "if this is a synthetic getter return the field" kind of logic that have nothing to do with navigation and could be simplified by such a getter.)

[jcollins-g]

That's even better. It's more general than my original proposal, though, so perhaps a more general name such as nonSyntheticElement would be in order. (There are probably plenty of places in analyzer, server and linter that do the "if this is a synthetic getter return the field" kind of logic that have nothing to do with navigation and could be simplified by such a getter.)

Yes, some places in dartdoc could be cleaned up a bit with this too.

Perhaps inducingElement or generatingElement or similar could be possible alternative names. Some way to convey "the element that caused this synthetic element to be created".

[bwilkerson]

... this synthetic element ...

Maybe I'm being too picky, but in case we're not thinking of the same thing ... I was thinking that we'd want this getter on all elements and that non-synthetic elements would return themselves. Then clients don't need to ask whether the element is synthetic before using the getter. (Similar to Element.declaration.)

[scheglov]

Well, inducingElement does not contradict the goal for it to return itself for non-synthetic elements, might be a matter of documenting it correspondingly.

Ideally I'd like to avoid using the word element, because we already return Element. It would be declaration, but we already use it.

[scheglov]

https://dart-review.googlesource.com/c/sdk/+/203080