Paul Berry (Gerrit)

unread,

Aug 8, 2025, 9:58:42 AM8/8/25

to Konstantin Shcheglov, Johnni Winther, Phil Quitslund, Brian Wilkerson, Commit Queue, dart-analys...@google.com, rev...@dartlang.org

Attention needed from Brian Wilkerson, Johnni Winther, Konstantin Shcheglov and Phil Quitslund

Paul Berry added 1 comment

Patchset-level comments

File-level comment, Patchset 4 (Latest):

Paul Berry . resolved

Requesting review from:

Konstantin or Johnni on changes to `pkg/analyzer`
Phil or Brian on changes to `pkg/linter` (Brian because he's been part of the design discussions for this change; Phil because Brian is out today)

Note that the changes to the `messages.yaml` files are large, but it should be fairly easy to have confidence in them, since the generated dart files are unchanged.

Open in Gerrit

Related details

Attention is currently required from:

Brian Wilkerson
Johnni Winther
Konstantin Shcheglov
Phil Quitslund

Submit Requirements:

Code-Owners
Code-Review
Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

unsatisfied_requirement

open

diffy

Johnni Winther (Gerrit)

unread,

Aug 8, 2025, 10:39:54 AM8/8/25

to Paul Berry, Konstantin Shcheglov, Phil Quitslund, Brian Wilkerson, Commit Queue, dart-analys...@google.com, rev...@dartlang.org

Attention needed from Brian Wilkerson, Konstantin Shcheglov, Paul Berry and Phil Quitslund

Johnni Winther voted and added 1 comment

Votes added by Johnni Winther

Code-Review

+1

1 comment

File pkg/analyzer/messages.yaml

Line 406, Patchset 4 (Latest): Element p1: the name of the first declaring extension

Johnni Winther . unresolved

For messages to be shareable between analyzer and CFE we need to limit kinds of types that we can use - or make some pseudo types. Passing in types can be called `Type` (as I see you did below) which translates into the two `DartType` classes in the analyzer and CFE, respectively. For `Element` it is a bit harder - it could be `Element` in the analyzer and `NamedBuilder` in the CFE, but I think that is a bit of a reach. Maybe it would be better to use `String` instead in cases where we want a name.

Open in Gerrit

Related details

Attention is currently required from:

Brian Wilkerson
Konstantin Shcheglov
Paul Berry
Phil Quitslund

Submit Requirements:

Code-Owners

Code-Review
Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

unsatisfied_requirement

satisfied_requirement

open

diffy

Paul Berry (Gerrit)

unread,

Aug 8, 2025, 11:52:07 AM8/8/25

to Johnni Winther, Konstantin Shcheglov, Phil Quitslund, Brian Wilkerson, Commit Queue, dart-analys...@google.com, rev...@dartlang.org

Attention needed from Brian Wilkerson, Johnni Winther, Konstantin Shcheglov and Phil Quitslund

Paul Berry added 1 comment

File pkg/analyzer/messages.yaml

Line 406, Patchset 4 (Latest): Element p1: the name of the first declaring extension

Johnni Winther . unresolved

For messages to be shareable between analyzer and CFE we need to limit kinds of types that we can use - or make some pseudo types. Passing in types can be called `Type` (as I see you did below) which translates into the two `DartType` classes in the analyzer and CFE, respectively. For `Element` it is a bit harder - it could be `Element` in the analyzer and `NamedBuilder` in the CFE, but I think that is a bit of a reach. Maybe it would be better to use `String` instead in cases where we want a name.

Paul Berry

Agreed that we need to limit the kinds of types we can use. I'm not 100% sure what to do about Element yet.

The problem with using `String` instead of `Element` is that the analyzer's error formatting logic includes special handling of ambiguous elements. If we changed the type to `String`, then the ambiguous element handling would have to be inlined at all call sites, and there would be a lot of risk that we would sometimes get it wrong or forget to do it.

I'll do some investigation on the CFE side and make a recommendation for how to move toward a shared design that preserves the ambiguous element handling functionality.

Open in Gerrit

Related details

Attention is currently required from:

Brian Wilkerson
Johnni Winther
Konstantin Shcheglov
Phil Quitslund

Submit Requirements:

Code-Owners
Code-Review
Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

unsatisfied_requirement

satisfied_requirement

open

diffy

Konstantin Shcheglov (Gerrit)

unread,

Aug 8, 2025, 12:25:45 PM8/8/25

to Paul Berry, Johnni Winther, Phil Quitslund, Brian Wilkerson, Commit Queue, dart-analys...@google.com, rev...@dartlang.org

Attention needed from Brian Wilkerson, Johnni Winther, Paul Berry and Phil Quitslund

Konstantin Shcheglov voted Code-Review+1

Code-Review

+1

Open in Gerrit

Related details

Attention is currently required from:

Brian Wilkerson
Johnni Winther
Paul Berry
Phil Quitslund

Submit Requirements:

Code-Owners
Code-Review
Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

unsatisfied_requirement

satisfied_requirement

open

diffy

Brian Wilkerson (Gerrit)

unread,

Aug 11, 2025, 12:48:26 PM8/11/25

to Paul Berry, Brian Wilkerson, Konstantin Shcheglov, Johnni Winther, Phil Quitslund, Commit Queue, dart-analys...@google.com, rev...@dartlang.org

Attention needed from Johnni Winther, Paul Berry and Phil Quitslund

Brian Wilkerson voted and added 2 comments

Votes added by Brian Wilkerson

Code-Review

+1

2 comments

Patchset-level comments

File-level comment, Patchset 5 (Latest):

Brian Wilkerson . resolved

The changes lgtm. I've sent an email to Parker (and included you on it). I hadn't thought about it before, but this might have implications for the way we generate documentation on `dart.dev`, so I wanted to get his input.

File pkg/analyzer/messages.yaml

Line 63, Patchset 5 (Latest): Object p0: the path of the file containing the error

Brian Wilkerson . unresolved

I'm guessing that future CLs will improve both the type and the name of these parameters. If that's not the case please let me know.

Open in Gerrit

Related details

Attention is currently required from:

Johnni Winther
Paul Berry
Phil Quitslund

Submit Requirements:

Code-Owners

Code-Review
Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

open

diffy

Phil Quitslund (Gerrit)

unread,

Aug 11, 2025, 1:29:05 PM8/11/25

to Paul Berry, Brian Wilkerson, Konstantin Shcheglov, Johnni Winther, Commit Queue, dart-analys...@google.com, rev...@dartlang.org

Attention needed from Johnni Winther and Paul Berry

Phil Quitslund voted Code-Review+1

Code-Review

+1

Open in Gerrit

Related details

Attention is currently required from:

Johnni Winther
Paul Berry

Submit Requirements:

Code-Owners
Code-Review
Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

open

diffy

Paul Berry (Gerrit)

unread,

Aug 11, 2025, 4:04:28 PM8/11/25

to Phil Quitslund, Brian Wilkerson, Konstantin Shcheglov, Johnni Winther, Commit Queue, dart-analys...@google.com, rev...@dartlang.org

Attention needed from Johnni Winther

Paul Berry voted and added 1 comment

Votes added by Paul Berry

Commit-Queue

+2

1 comment

File pkg/analyzer/messages.yaml

Line 63, Patchset 5 (Latest): Object p0: the path of the file containing the error

Brian Wilkerson . resolved

I'm guessing that future CLs will improve both the type and the name of these parameters. If that's not the case please let me know.

Paul Berry

Yes, my plan was to gradually improve these types and names as part of the process of converting the analyzer over to report the errors using the new (as yet unimplemented) literate API for reporting errors.

I'm also open to contributions if you (or anyone else) would like to take a stab at improving some of them in the meantime 😊

Open in Gerrit

Related details

Attention is currently required from:

Johnni Winther

Submit Requirements:

Code-Owners
Code-Review
Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

open

diffy

Commit Queue (Gerrit)

unread,

Aug 11, 2025, 4:29:40 PM8/11/25

to Paul Berry, Phil Quitslund, Brian Wilkerson, Konstantin Shcheglov, Johnni Winther, dart-analys...@google.com, rev...@dartlang.org

Commit Queue submitted the change

Change information

Commit message:

[analyzer/linter] Use structured parameter format in messages.yaml.

This commit changes the format of `messages.yaml`. Message parameters
are now defined in a structured `parameters` section, replacing the
old free-form comments. This new format explicitly names and types
each parameter, which will allow for future improvements to static
error checking.

Also, the placeholder syntax is changed from `{N}` to `#NAME` to align
with the CFE, which will facilitate sharing error messages between the
analyzer and CFE in the future.

For now, the parameter names and types have been chosen fairly
arbitrarily; I plan to refine them in future CLs, working with small
sets of error messages at a time.

Additionally, the code generator now documents the parameters in a new
`Parameters:` section at the end of each error code's doc
comment. This change doesn't alter the generated files for now (to
give extra confidence that the change won't affect behavior). In a
follow-up, I plan to update the generated comments to include the
parameter names and types.

Change-Id: Ia704e2c30d466b379b956ba8bbcee8bbef7bf11a

Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/444388

Commit-Queue: Paul Berry <paul...@google.com>

Reviewed-by: Brian Wilkerson <brianwi...@google.com>

Reviewed-by: Johnni Winther <johnni...@google.com>

Reviewed-by: Konstantin Shcheglov <sche...@google.com>

Reviewed-by: Phil Quitslund <pquit...@google.com>

Files:

M pkg/analyzer/messages.yaml
M pkg/analyzer/tool/messages/error_code_info.dart
M pkg/analyzer/tool/messages/generate.dart
M pkg/linter/messages.yaml

Change size: XL

Delta: 4 files changed, 2891 insertions(+), 3430 deletions(-)

Branch: refs/heads/main

Submit Requirements:

Code-Review: +1 by Konstantin Shcheglov, +1 by Johnni Winther, +1 by Phil Quitslund, +1 by Brian Wilkerson

Open in Gerrit

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

open

diffy

satisfied_requirement

Reply all

Reply to author

Forward

[XL] Change in dart/sdk[main]: [analyzer/linter] Use structured parameter format in messages.yaml.

Paul Berry (Gerrit)

Paul Berry added 1 comment

Related details

Johnni Winther (Gerrit)

Johnni Winther voted and added 1 comment

Votes added by Johnni Winther

1 comment

Related details

Paul Berry (Gerrit)

Paul Berry added 1 comment

Related details

Konstantin Shcheglov (Gerrit)

Konstantin Shcheglov voted Code-Review+1

Related details

Brian Wilkerson (Gerrit)

Brian Wilkerson voted and added 2 comments

Votes added by Brian Wilkerson

2 comments

Related details

Phil Quitslund (Gerrit)

Phil Quitslund voted Code-Review+1

Related details

Paul Berry (Gerrit)

Paul Berry voted and added 1 comment

Votes added by Paul Berry

1 comment

Related details

Commit Queue (Gerrit)

Commit Queue submitted the change

Change information