[ty] Add contextual secondary annotations in more places

[AlexWaygood]

Summary

Following #24689, it's now much more likely that a diagnostic regarding an invalid @final, for example, will not include the @final decorator in the code frame.

This was already a pre-existing issue, because there's nothing to stop a class from having multiple decorators, so @final was really only "accidentally" in the context window most of the time. E.g. for this class, @final would not have been in the context window even with our old context margin of 2:

from typing import final
from abc import abstractmethod
from dataclasses import dataclass
from functools import total_ordering

@final
@dataclass
@total_ordering
class Foo:
    @abstractmethod
    def __lt__(self, other): ...

Our diagnostic on the latest release of ty for that was:

But #24689 makes this problem much likelier to occur. This PR therefore adds secondary annotations to several diagnostics that ensure that relevant decorators are included in the diagnostic frames rendered to the user

Test plan

snapshots updated

[astral-sh-bot]

Typing conformance results

No changes detected ✅

Current numbers

The percentage of diagnostics emitted that were expected errors held steady at 87.94%. The percentage of expected errors that received a diagnostic held steady at 83.36%. The number of fully passing files held steady at 79/133.

[astral-sh-bot]

Memory usage report

Memory usage unchanged ✅

[MichaReiser]

Showing the @abstractmethod is great! But I wonder if the ---- without any trailing text is confusing. Should we say something like *method is abstract because of the abstractmethod decorator? Curious to hear if others feel the same.

[AlexWaygood]

Hmm, I'm not sure. To me it feels pretty clear why we're highlighting it, given that the primary annotation already says that the issue is that method is an abstract classmethod? And the name of the decorator is pretty clear. Overloading the frame with too many labels also has a cost -- it makes it harder to spot the more important information in the diagnostic.

I feel like I've seen Clippy do similar things with secondary annotations in the past, too.

Here's what it looks like with a bit of colour:

[MichaReiser]

This example makes me wonder if it's now also necessary to underline raise NotImplementedError or the "trivial body" part isn't visible in the code frame.

Which makes me wonder if the code frame should spawn the entire method (which is generally bad?). You then only need one annotation and it's guaranteed that you see all relevant details.

[AlexWaygood]

That's a great point. Since we know this method has a trivial body in this situation, it's very unlikely that the method body will be huge. That makes a lot of sense

[MichaReiser]

Do you plan on changing this or did it turn out that it regresses the experience in some cases?

[AlexWaygood]

I changed it in cad725c

[MichaReiser]

Ohh, that's different than what I had in mind. I would used the entire func_dev.range as the primary range.

[AlexWaygood]

yeah, that might be better.

[astral-sh-bot]

ecosystem-analyzer results

No diagnostic changes detected ✅

Flaky changes detected. This PR summary excludes flaky changes; see the HTML report for details.

Full report with detailed diff (timing results)

[MichaReiser]

Should we make the same change here?

[AlexWaygood]

No, because here the stub body of the abstractmethod is irrelevant to the diagnostic. In this case, we would still emit the diagnostic if the function body was return 42 rather than raise NotImplementedError, and in this case we do not say anything about how the method has a stub body in the diagnostic message

[MichaReiser]

Ohh, that's different than what I had in mind. I would used the entire func_dev.range as the primary range.