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

CFE code should start passing a language version to dart_style #56687

Closed
Tracked by #56688
munificent opened this issue Sep 10, 2024 · 3 comments
Closed
Tracked by #56688

CFE code should start passing a language version to dart_style #56687

munificent opened this issue Sep 10, 2024 · 3 comments
Assignees
Labels
area-front-end Use area-front-end for front end / CFE / kernel format related issues. cfe-testing Issues related to testing of the CFE.

Comments

@munificent
Copy link
Member

munificent commented Sep 10, 2024

Greetings CFE 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/front_end/benchmarks/patterns/generate_datatypes.dart:
   81:   String result = new DartFormatter().format(sb.toString());
  200:   String result = new DartFormatter().format(sb.toString());

~/dev/dart/sdk/pkg/front_end/test/parser_test_listener_creator.dart:
  117:   return new DartFormatter().format("$out");

~/dev/dart/sdk/pkg/front_end/test/parser_test_parser_creator.dart:
  118:   return new DartFormatter().format("$out");

~/dev/dart/sdk/pkg/front_end/test/fasta/textual_outline_suite.dart:
  160:           result = new DartFormatter(experimentFlags: experimentFlags)

~/dev/dart/sdk/pkg/front_end/tool/visitor_generator.dart:
   74:     result = new DartFormatter().format(result);

~/dev/dart/sdk/pkg/front_end/tool/_fasta/generate_experimental_flags.dart:
  182:   return new DartFormatter().format("$sb");
  216:   return new DartFormatter().format("$sb");
  528:   return new DartFormatter().format("$sb");

~/dev/dart/sdk/pkg/front_end/tool/_fasta/generate_messages.dart:
   31:       repoDir, (s) => new DartFormatter().format(s));

~/dev/dart/sdk/pkg/front_end/tool/_fasta/parser_ast_helper_creator.dart:
  122:   return new DartFormatter().format("$out");

Thank you!

@munificent munificent added the area-front-end Use area-front-end for front end / CFE / kernel format related issues. label Sep 10, 2024
@munificent
Copy link
Member Author

I have a review out that addresses most of them: https://dart-review.googlesource.com/c/sdk/+/385341

The remaining one is textual_outline_suite.dart. That one probably should pass in a correct language version, and I don't know enough of the surrounding context to know how to get that.

copybara-service bot pushed a commit that referenced this issue Sep 17, 2024
As far as I can tell, these scripts are all only used internally for
formatting generated code, so it should be safe to just parse the code
at the latest language version.

I didn't migrate:

  pkg/front_end/test/fasta/textual_outline_suite.dart

I believe that one may need to pass in a specific language version if it
needs to support formatting code from before Dart 3.0. (In particular
if it needs to handle code using old switch constant expressions that
are not supported in Dart 3.0 and later like `case 1 + 2:`.)

Bug: #56687
Change-Id: Ib5cef82c22fc749223095215d3b692e6c27decc7
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/385341
Reviewed-by: Jens Johansen <jensj@google.com>
Commit-Queue: Bob Nystrom <rnystrom@google.com>
Auto-Submit: Bob Nystrom <rnystrom@google.com>
@munificent
Copy link
Member Author

OK, all that's left is:

~/dev/dart/sdk/pkg/front_end/test/fasta/textual_outline_suite.dart:
  160:           result = new DartFormatter(experimentFlags: experimentFlags)

@johnniwinther johnniwinther added the cfe-testing Issues related to testing of the CFE. label Sep 20, 2024
copybara-service bot pushed a commit that referenced this issue Oct 11, 2024
There is an upcoming version of dart_style that makes this argument
required, so every DartFormatter() constructor callsite needs to pass
it in. These are the last two I could find in the Dart SDK.

I'm not familiar with the surrounding code, but I believe passing in
DartFormatter.latestLanguage version will do the right thing:

* For the benchmark script, that's generated code that we will always
  want to be valid on the latest version of Dart.

* For the text_outline_suite, my understanding is that CFE tests that
  require older language versions always use the "// @Dart=x.y" comment
  to opt in to that version. Those comments are also respected by the
  formatter and override the languageVersion argument passed to the
  constructor. So I think passing in latestSupportedVersion will do the
  right thing.

Bug: #56687

Change-Id: If29188df479c7b41beb68ac889d56be13a395a16
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/389622
Commit-Queue: Johnni Winther <johnniwinther@google.com>
Auto-Submit: Bob Nystrom <rnystrom@google.com>
Reviewed-by: Johnni Winther <johnniwinther@google.com>
@munificent
Copy link
Member Author

This is done now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-front-end Use area-front-end for front end / CFE / kernel format related issues. cfe-testing Issues related to testing of the CFE.
Projects
None yet
Development

No branches or pull requests

3 participants