Lints and macro generated code

[jakemac53]

It isn't clear whether lints should apply to macro generated code or not, or whether the lint authors, lint users, or both should have the ability to control whether or not they apply to generated code.

This came up in the context of #3345 - we have a lint that would require an explicit @override annotation for overrides. So it would trigger if a macro created an override when it thought it was creating a new field/method/etc. Thus, quite possibly that lint should run on generated code. But that may not be true for all lints.

[rrousselGit]

In my opinion, only macro authors care about lints in generated code.

To begin with, there are conflicting rules. Things like "prefer single quote strings" vs "prefer double quote strings" are opinionated and would put too much burden on macro authors to respect.
But during development of a macro, macro authors do want to see lints. They could help spot cases where generated code use deprecated members, or possible optimisations such as using a tear-off when doable.

In all of my code-generators, I have the generated code include // ignore_for_file: type=lint to disable all lints. And I may manually remove this line during developpement to see if there's something I forgot.

[srawlins]

CC @bwilkerson @scheglov

We discussed this a bit briefly and I put forward the idea that we can report diagnostics in (generated) augmentations only if the file is opened in the editor. This is how we treat "excluded" files. An excluded file's problems are not reported in the Problems panel of an editor, but if you open an excluded file, then you get all the analysis, Problems panel, highlighting, red squigglies, etc.

I think the biggest motivation for not reporting lint is that there are plenty of style-motivated lints, as you noted, and the author of a generated augmentation cannot know or follow the style conventions of the package into which their code is being generated.

CC @pq We might be able to up our game in the categorization of lints, such that a line like // ignore_for_file: type=style-motivated lint could work.

[jakemac53]

In my opinion, only macro authors care about lints in generated code.

I do generally agree with this - or at least have historically - but I do think that annotate_overrides as an example would be useful to users of macros, in the general case. It would indicate to them that a macro was probably doing something unexpected (by both them and you).

[bwilkerson]

It would indicate to them that a macro was probably doing something unexpected (by both them and you).

I think that's the salient point here. As a user I would want to know if a macro was generating code that was breaking my code in some way. Generating an override of a member that I didn't expect to be overridden is one possible example, but I am sure that there are others.

The hard part is deciding which lints and warnings indicate a possible breakage. It's easy (and valid, I think) to assume that stylistic lints don't. But exactly which lints and warnings might is a harder task.

If all macro authors are careful to proactively check for such issues and generate diagnostics of their own when the issues occur, then I think there wouldn't be any question that additional lints and warnings shouldn't be reported. But while I hope that macro authors do this extra work, I'm realistic enough to know that not all of them will. And nobody, however well intentioned, is perfect.

I think it's important that we make it as easy as we can for macro users to find the cause of macro-related bugs. Reporting some subset of lints and warnings might be a part of that.

I'll also point out that it's possible that we don't want to report these diagnostics in the generated code, but to instead associate them with the macro invocation, given that the invocation is the only place where the user can make any changes that might be necessary.

[rrousselGit]

One alternative is, we could have "infos don't show up, but warnings and errors do".
And we could promote some "info" lints (such as annotate_overrides) to warnings.

Otherwise the idea of cherry picking lints sounds a bit tricky to me. It would likely be confusing about what some lints how-up but not others.

Another thing to keep in mind is: Analyzer plugins.
While currently few people make custom lint rules, that's bound to change as Dart grows.
A rule such as "warnings should be visible but not infos" would handle custom warnings too.

[pq]

I'll also point out that it's possible that we don't want to report these diagnostics in the generated code, but to instead associate them with the macro invocation, given that the invocation is the only place where the user can make any changes that might be necessary.

I've wondered this exact thing.

Otherwise the idea of cherry picking lints sounds a bit tricky to me.

I agree here too. For this we'd really want to support user-choice. The most natural way for that might be
with a richer configuration of analysis options (as discussed in dart-lang/sdk#57673 for example).

One alternative is, we could have "infos don't show up, but warnings and errors do".

What's cool about this idea is that we can approximate some configuration with what we have today. If someone wanted to escalate style lints in generated code they could up their severity in options.

[bwilkerson]

One alternative is, we could have "infos don't show up, but warnings and errors do".

If someone wanted to escalate style lints in generated code they could up their severity in options.

True, but there might be other implications of changing the severity, such causing a CI system to start failing.

It seems to me that using the severity to indicate whether the diagnostic ought to be reported for generated code is conflating two fairly different signals.

[rrousselGit]

There have been info lints who were escalated to warnings not too long ago. I don't think doing it for a few more would be an issue.
And Flutter treats infos as failures by default anyway. It's one of the difference between dart analyze and flutter analyze. So I don't think it'd be a huge deal

[jakemac53]

I don't mind the "show warnings but not lints" approach, I think it's pretty reasonable. We don't care about style, we care about potentially unsafe things. And as noted above we already have a way to change the severity of a specific lint which allows for user configuration.

Also, I think if somebody cares enough to promote a lint to a warning, they also should want their CI to fail if any of those warnings are present.

[jakemac53]

Also, we need to discuss how ignoring these lints/warnings should work. Probably, the ignore should go on the macro annotation? It can't go on the generated code because users can't add that.