Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Analyzer code should start passing a language version to dart_style #56685

Open
6 of 9 tasks
Tracked by #56688
munificent opened this issue Sep 10, 2024 · 2 comments
Open
6 of 9 tasks
Tracked by #56688

Analyzer code should start passing a language version to dart_style #56685

munificent opened this issue Sep 10, 2024 · 2 comments
Assignees
Labels
area-analyzer Use area-analyzer for Dart analyzer issues, including the analysis server and code completion. P2 A bug or feature request we're likely to work on type-enhancement A request for a change that isn't a bug

Comments

@munificent
Copy link
Member

munificent commented Sep 10, 2024

Greetings analyzer team friends!

The formatter is moving to being language version aware. This means that when it's parsing some code, it needs to be told what language version to parse it as. The DartFormatter constructor now takes an optional parameter where you can pass in the language version. In a future version of dart_style, that parameter will become mandatory.

If you own code that uses dart_style as a library, you should update your code to pass in a language version. To migrate:

  1. If this code is inside a package (as opposed to, say, the Dart core libraries), then update your constraint on dart_style to ^2.3.7. That's the version where the new parameter was added.

  2. If you know the precise language version the code should be parsed as, then pass in that version (as an instance of the pub_semver package's Version class). This is what "real" tools should do.

    For simple one-off scripts and other utilities where precise behavior doesn't matter much, you can pass in DartFormatter.latestLanguageVersion to unconditionally parse the code as the latest language version that the formatter itself supports.

When the --tall-style experiment flag ships, the passed in language version will also be used to determine whether you get the current formatting style or the new "tall" style.

I did a search through the SDK, and the constructor calls that I think are relevant are:

~/dev/dart/sdk/pkg/analysis_server/lib/src/g3/utilities.dart:
   24:   var formatter = DartFormatter();

~/dev/dart/sdk/pkg/analysis_server/lib/src/handler/legacy/edit_format.dart:
   51:     var formatter = DartFormatter(pageWidth: params.lineLength);

~/dev/dart/sdk/pkg/analysis_server/lib/src/handler/legacy/edit_format_if_enabled.dart:
   34:     var formatter = DartFormatter();

~/dev/dart/sdk/pkg/analysis_server/lib/src/lsp/source_edits.dart:
   95:     formattedResult = DartFormatter(pageWidth: lineLength).formatSource(code);

~/dev/dart/sdk/pkg/analysis_server/tool/lsp_spec/codegen_dart.dart:
   17: final formatter = DartFormatter();

~/dev/dart/sdk/pkg/analyzer_plugin/lib/src/utilities/change_builder/change_builder_dart.dart:
 1502:     var formattedResult = DartFormatter().formatSource(

Thank you!


Implementation:

  • migrate g3/utilities.dart
  • migrate legacy/edit_format.dart
  • migrate legacy/edit_format_if_enabled.dart
  • migrate lsp/source_edits.dart
  • migrate lsp_spec/codegen_dart.dart
  • migrate change_builder/change_builder_dart.dart

Testing:

Testing is currently tricky as the formatter is not yet gating tall-style on language version (requiring an experiment flag in the meantime). We'll want to loop back add tests once that work is complete.

  • update edit_format_test.dart to test tall/short styles
  • update format_test.dart to test tall/short styles
  • update source_edits_test.dart to test tall/short styles
@munificent munificent added the area-analyzer Use area-analyzer for Dart analyzer issues, including the analysis server and code completion. label Sep 10, 2024
@pq pq added type-enhancement A request for a change that isn't a bug P2 A bug or feature request we're likely to work on labels Sep 10, 2024
copybara-service bot pushed a commit that referenced this issue Sep 25, 2024
See: #56685


Change-Id: Ifa7fbbc7aff7c5fd96b02cafe0da76b22a33b1e9
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/386824
Commit-Queue: Phil Quitslund <[email protected]>
Reviewed-by: Brian Wilkerson <[email protected]>
@pq pq self-assigned this Sep 26, 2024
@pq
Copy link
Member

pq commented Sep 26, 2024

FYI @DanTup, @bwilkerson: as per a conversation with @munificent, formatting is not yet being gated by version so testing is complicated. As suggested above, I propose we leave tests as a TODO for now and fill them in once the formatter work is complete.

copybara-service bot pushed a commit that referenced this issue Sep 26, 2024
(Testing to follow.)

See: #56685




Change-Id: I9da0e3868cd3c873c41e8a19a77fad58b9b8dea0
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/386901
Reviewed-by: Brian Wilkerson <[email protected]>
Commit-Queue: Phil Quitslund <[email protected]>
copybara-service bot pushed a commit that referenced this issue Sep 26, 2024
Chatting with Bob, it turns out `languageVersion` when required will be non-nullable.

This change anticipates that.


Bug: #56685
Change-Id: I53ec5b4ed408197b6b99871bf176e6c6cc9654dd
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/387142
Commit-Queue: Phil Quitslund <[email protected]>
Reviewed-by: Brian Wilkerson <[email protected]>
@munificent
Copy link
Member Author

munificent commented Oct 2, 2024

Testing is currently tricky as the formatter is not yet gating tall-style on language version (requiring an experiment flag in the meantime). We'll want to loop back add tests once that work is complete.

In terms of separation of concerns, it's not really analyzer's job to validate which formatting style you get. That's the formatter's job. All this issue is about is ensuring that you're passing the correct language version to the formatter. You can test that by trying to format syntax that is only valid at certain language versions. So:

  • If you pass in 2.19, then switch (o) { case 1 + 2: break; } should be OK but var (a, b) = (1, 2); should report a parse error.
  • Conversely, if you pass in 3.0, then switch (o) { case 1 + 2: break; } should be a parse error but var (a, b) = (1, 2); should be OK.

Thank you for working on this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-analyzer Use area-analyzer for Dart analyzer issues, including the analysis server and code completion. P2 A bug or feature request we're likely to work on type-enhancement A request for a change that isn't a bug
Projects
None yet
Development

No branches or pull requests

2 participants