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

[breaking change] Remove the colon-syntax for default values. #2357

Closed
lrhn opened this issue Jul 28, 2022 · 28 comments
Closed

[breaking change] Remove the colon-syntax for default values. #2357

lrhn opened this issue Jul 28, 2022 · 28 comments
Assignees
Labels
feature Proposed language feature that solves one or more problems

Comments

@lrhn
Copy link
Member

lrhn commented Jul 28, 2022

For historical reasons, named optional parameters can specify their default value using either : or =.
The = was introduced later because it's better. It's probably about time to remove the ability to use :.
So, we should do so for Dart 3.0.

Example:

int someInt({int x: 0}) => x;

is currently valid, will become invalid.

We have a lint recommending using =, and it's in our recommended set, so there should hopefully be very few remaining uses, most likely only in code so old it's not null-safe anyway.

Might also open up the syntax for something else, even if I don't know what. (Types on the right, anybody?)

@lrhn lrhn added the feature Proposed language feature that solves one or more problems label Jul 28, 2022
@mit-mit
Copy link
Member

mit-mit commented Aug 8, 2022

@bwilkerson does dart fix already update these from using : to using =? If not, how much would work that be to support?

@leafpetersen
Copy link
Member

We may wish to start emitting a warning in the next release that this is going away.

@bwilkerson
Copy link
Member

Sorry I missed the ping.

dart fix will currently only fix these is the lint prefer_equal_for_default_values is enabled.

Sounds like we should convert the lint to a warning so that it's always enabled.

I think the right path forward is to mark the lint as deprecated when the warning is implemented. Doing so will enable a fix to remove the lint from the analysis options file.

@kevmoo
Copy link
Member

kevmoo commented Aug 10, 2022

dartfmt was the tool that did this fix IIRC

@mit-mit
Copy link
Member

mit-mit commented Aug 11, 2022

Sounds like we should convert the lint to a warning so that it's always enabled.

Yes, I think a roll-out plan like this might work:

  • Current: flagged by lint prefer_equal_for_default_values, which is part of lints/recommended.yaml
  • Dart 2.19: make it a warning in the analyzer; deprecate the lint
  • Dart 3: make it an error generally, remove the lint

@bwilkerson does that sound right?

And if we do so, would a developer using 2.19 who for some reason want to ignore the warning be able to suppress it with an ignore in their analysis config?

@bwilkerson
Copy link
Member

That sounds right to me. I assume that we'll put out a breaking change notice before 2.19 and again before 3.0.

And if we do so, would a developer using 2.19 who for some reason want to ignore the warning be able to suppress it with an ignore in their analysis config?

Yes. Warnings can be ignored via the analysis options file or by using an ignore comment.

I have a CL to add the warning and am using it to gauge the amount of work that will be required to land it. I don't know what the time frame for 2.19 looks like, so I don't know when this needs to land.

@mit-mit
Copy link
Member

mit-mit commented Aug 11, 2022

Thanks! 2.19 branching is several months away.

@lrhn
Copy link
Member Author

lrhn commented Aug 11, 2022

I noticed that we have a lot of occurrences in the platform libraries (which includes some of the oldest Dart code in existence, so not entirely surprising). Some clean-up will definitely be needed.

Most can be handled by a dart format --fix-named-default-separator run, but a few are hiding in strings used to generate the code.
(https://dart-review.googlesource.com/c/sdk/+/254700)

@bwilkerson
Copy link
Member

I noticed the same when I ran a dry-run on the CL that adds the analyzer support (https://dart-review.googlesource.com/c/sdk/+/254467).

@bwilkerson
Copy link
Member

As can be seen from the CBuild failure on https://dart-review.googlesource.com/c/sdk/+/254467, there are still a fair number of uses of colon-syntax internally. We probably want to get the lint enabled internally as a path toward being able to land this change.

@mit-mit
Copy link
Member

mit-mit commented Aug 16, 2022

This would probably be useful to get the code migrated: dart-lang/sdk#47219

@mit-mit
Copy link
Member

mit-mit commented Sep 7, 2022

@bwilkerson we've been doing a ton of cleanup. Can you try to run your CL checks again after rebasing?

@parlough
Copy link
Member

parlough commented Sep 26, 2022

I'm not sure the timeline for the analyzer warning, but perhaps prefer_equal_for_default_values should be moved to the core lints set rather than recommended? At least if the next release of lints is intended before that warning.

@kevmoo
Copy link
Member

kevmoo commented Sep 26, 2022

Great suggestion, @parlough !!

@mit-mit
Copy link
Member

mit-mit commented Sep 29, 2022

I like that in spirit, but I think practically only pretty old Dart code is likely to use this style, and I doubt any of that is a) configured to use our new lints, and b) would upgrade to a new version of those lints. So I speculate that it would make no practical difference?

github-actions bot pushed a commit to gnoliyil/fuchsia that referenced this issue Oct 10, 2022
Dart currently allows for two different syntaxes for default
values. One is getting deprecated:
dart-lang/language#2357

This CL updates fidlgen to no longer use the
about-to-be-deprecated syntax.

Change-Id: I870e839450b37c51643808a15b0dc9d1fcf7f044
Reviewed-on: https://fuchsia-review.googlesource.com/c/fuchsia/+/737404
Reviewed-by: Alex Zaslavsky <azaslavsky@google.com>
Reviewed-by: Mitchell Kember <mkember@google.com>
Commit-Queue: Michael Thomsen <mit@google.com>
@bwilkerson
Copy link
Member

The analyzer changes have now landed.

@bwilkerson bwilkerson removed their assignment Nov 7, 2022
@mit-mit
Copy link
Member

mit-mit commented Nov 21, 2022

@bwilkerson can I get you to create a second changelist for the 3.0 alpha change, which makes this an error and removes the lint? This CL would be landed right after we've done the last branch point for 2.19 and the main branch is open for 3.0 development.

@mit-mit mit-mit changed the title Remove the colon-syntax for default values. [breaking change] Remove the colon-syntax for default values. Nov 22, 2022
@mit-mit
Copy link
Member

mit-mit commented Nov 28, 2022

Perhaps the Dart 3 alpha change we can split into two steps: a) make it an error, and b) remove the lint. I think the former is the most urgent.

@bwilkerson
Copy link
Member

... can I get you to create a second changelist for the 3.0 alpha change ...

Sorry I missed the ping again. Yes.

Perhaps the Dart 3 alpha change we can split into two steps: a) make it an error, and b) remove the lint.

Yes, it will likely be two steps. We're trying to deprecate the lint before 2.19 so that users will know that it's no longer needed and will be removed.

copybara-service bot pushed a commit to dart-lang/sdk that referenced this issue Nov 30, 2022
The colons cause test failures when the language version is bumped to 3.0. This CL can be landed before the 3.0 version bump.

Bug: dart-lang/language#2357
Change-Id: Id8396034b16adc18b476689314e28b9617d25f18
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/272200
Commit-Queue: Alexander Thomas <athom@google.com>
Reviewed-by: Daco Harkes <dacoharkes@google.com>
@mit-mit
Copy link
Member

mit-mit commented Dec 5, 2022

@bwilkerson any updates on the CL to make this an error? I don't see it here:
https://dart-review.git.corp.google.com/q/message:%2522%255B3.0+alpha%255D%2522

@bwilkerson
Copy link
Member

It should happen automatically with the change in the SDK version number. There are changes in that CL to fix some tests that were reporting the warning to make them expect the error, so I assumed that everything was working correctly. To the best of my knowledge, there is no further work to be done and we can close this issue once that CL has landed.

@mit-mit
Copy link
Member

mit-mit commented Dec 6, 2022

Indeed that is the case 🥳 :

$ dart --version
Dart SDK version: 3.0.0-edge.606a64a743b80e5f21d7818e0d4119376083e1d1 (be) (Tue Dec 6 04:04:23 2022 +0000) on "macos_arm64"


$ cat bin/colons.dart
import 'package:colons/colons.dart' as colons;

int someInt({int x: 0}) => x;


$ dart analyze .
Analyzing ....                         0.7s

  error • bin/colons.dart:3:19 • Using a colon as a separator before a default value is no longer supported. Try replacing the colon with an equal sign. •
          obsolete_colon_for_default_value

1 issue found.

Thanks!

@mit-mit mit-mit closed this as completed Dec 6, 2022
@vsmenon vsmenon reopened this Feb 28, 2023
@vsmenon
Copy link
Member

vsmenon commented Feb 28, 2023

@lrhn - the analyzer reports this as an error now, but the CFE does not. Shouldn't the CFE as well?

If so, can you please open an issue on the CFE for Dart 3?

@vsmenon vsmenon assigned lrhn and unassigned bwilkerson and mit-mit Feb 28, 2023
@vsmenon
Copy link
Member

vsmenon commented Feb 28, 2023

FYI - @mit-mit @itsjustkevin

@munificent
Copy link
Member

The CFE issue is here.

Echoing a comment there on the main issue here for posterity: This is a language versioned changed. It's an error to use : as a default value separator in a Dart 3.0 library, but not in a library whose language version is <3.0.

@mit-mit
Copy link
Member

mit-mit commented Mar 15, 2023

The CFE change handled in dart-lang/sdk@bd02ce7.

I'm not aware of any remaining work. @munificent can we go ahead and close this given all the changes are in main?

@munificent
Copy link
Member

I think so. This change is @lrhn's baby, but as far as I know, it's done.

@mit-mit
Copy link
Member

mit-mit commented Mar 16, 2023

Closing as there is no known work

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Proposed language feature that solves one or more problems
Projects
None yet
Development

No branches or pull requests

8 participants