language icon indicating copy to clipboard operation
language copied to clipboard

Remove the colon-syntax for default values.

Open lrhn opened this issue 1 year ago • 12 comments

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 avatar Jul 28 '22 10:07 lrhn

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

mit-mit avatar Aug 08 '22 10:08 mit-mit

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

leafpetersen avatar Aug 10 '22 15:08 leafpetersen

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.

bwilkerson avatar Aug 10 '22 15:08 bwilkerson

dartfmt was the tool that did this fix IIRC

kevmoo avatar Aug 10 '22 21:08 kevmoo

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?

mit-mit avatar Aug 11 '22 12:08 mit-mit

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.

bwilkerson avatar Aug 11 '22 14:08 bwilkerson

Thanks! 2.19 branching is several months away.

mit-mit avatar Aug 11 '22 14:08 mit-mit

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)

lrhn avatar Aug 11 '22 15:08 lrhn

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 avatar Aug 11 '22 15:08 bwilkerson

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.

bwilkerson avatar Aug 12 '22 15:08 bwilkerson

This would probably be useful to get the code migrated: https://github.com/dart-lang/sdk/issues/47219

mit-mit avatar Aug 16 '22 08:08 mit-mit

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

mit-mit avatar Sep 07 '22 20:09 mit-mit

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.

parlough avatar Sep 26 '22 22:09 parlough

Great suggestion, @parlough !!

kevmoo avatar Sep 26 '22 22:09 kevmoo

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?

mit-mit avatar Sep 29 '22 15:09 mit-mit

The analyzer changes have now landed.

bwilkerson avatar Nov 07 '22 21:11 bwilkerson

@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 avatar Nov 21 '22 15:11 mit-mit

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.

mit-mit avatar Nov 28 '22 20:11 mit-mit

... 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.

bwilkerson avatar Nov 28 '22 21:11 bwilkerson

@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

mit-mit avatar Dec 05 '22 08:12 mit-mit

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.

bwilkerson avatar Dec 05 '22 15:12 bwilkerson

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 avatar Dec 06 '22 15:12 mit-mit

@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 avatar Feb 28 '23 14:02 vsmenon

FYI - @mit-mit @itsjustkevin

vsmenon avatar Feb 28 '23 14:02 vsmenon

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.

munificent avatar Mar 01 '23 19:03 munificent

The CFE change handled in https://github.com/dart-lang/sdk/commit/bd02ce7a7f16408b99d04fee2d758e1fbe96dabe.

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

mit-mit avatar Mar 15 '23 22:03 mit-mit

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

munificent avatar Mar 16 '23 20:03 munificent

Closing as there is no known work

mit-mit avatar Mar 16 '23 20:03 mit-mit