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

Require the first argument to assert() to be a bool #30326

Closed
12 tasks done
munificent opened this issue Aug 4, 2017 · 14 comments
Closed
12 tasks done

Require the first argument to assert() to be a bool #30326

munificent opened this issue Aug 4, 2017 · 14 comments
Assignees
Labels
area-language Dart language related items (some items might be better tracked at github.com/dart-lang/language).
Milestone

Comments

@munificent
Copy link
Member

munificent commented Aug 4, 2017

The language currently allows the first argument to assert() to be either a bool, or a nullary function that returns a bool when called. In the latter case, it is implicitly invoked:

assert(() {
  return true;
});

That only adds the tiniest bit of syntactic sugar. Without this feature, you could always just call the function yourself:

assert(() {
  return true;
}());

In return for saving you a (), the type inference for assert() isn't as useful as it could be. It can't assume that the first argument should be a bool when performing downwards type inference.

There is also, of course, the implementation complexity and cognitive overhead of this obscure feature. It doesn't carry its weight, so the language team has decided to remove it for Dart 2.0.

In 2.0, the first argument to assert() must be an expression that evaluates to true or false. You can migrate your existing using of the removed feature by adding a () after your function.

We still need to figure out what tasks are required to ship this, but to get started:

  • Update the assert() tests. (https://dart-review.googlesource.com/c/sdk/+/4181)
  • Remove support for it from analyzer.
  • Remove support for it from dart2js.
  • Remove support for it from dartdevc. (https://dart-review.googlesource.com/c/sdk/+/4182)
  • Remove support for it from the front-end.
  • Remove support for it from the VM.
  • Remove uses of it in the core libraries and packages we maintain.
  • Remove uses of it inside google.
  • Announce this publicly and explain the migration path.
  • Update documentation that mentions this (if any).
  • Remove it from the spec. (https://codereview.chromium.org/2974763002)
  • Update downwards type inference in analyzer and front_end to assume the first argument to assert() is a bool.
@munificent munificent added the area-language Dart language related items (some items might be better tracked at github.com/dart-lang/language). label Aug 4, 2017
@munificent munificent added this to the 2.0 milestone Aug 4, 2017
@munificent munificent self-assigned this Aug 4, 2017
@bwilkerson
Copy link
Member

Remove support for it from analyzer.

What this probably means in practice is to start issuing a hint relatively soon indicating that this feature is obsolete (with a quick fix to add the parentheses). In 2.0 we can promote the hint to an error.

@matanlurey
Copy link
Contributor

For google, we're going to want it to be an error sooner, i.e. to prevent slide-back.

@jmesserly
Copy link

For google, we're going to want it to be an error sooner, i.e. to prevent slide-back.

we can have it be an error in strong mode. I'm happy to send CL's to help fix internal code for the Dart SDK roll

@matanlurey
Copy link
Contributor

Woohoo!

@bwilkerson
Copy link
Member

Last I heard, hints will also fail the internal build, so I don't think promoting it sooner will be necessary.

stereotype441 added a commit that referenced this issue Aug 7, 2017
This addresses the type inference part of #30326.

R=scheglov@google.com

Review-Url: https://codereview.chromium.org/2997513002 .
whesse pushed a commit that referenced this issue Sep 14, 2017
Dart used to allow the first argument to assert() to be a function,
which it would implicitly call, but this is being removed for Dart 2.0.

#30326

This gets the test testing the 2.0 behavior. Since the test is now
leading the implementation, updated the status files to mark it failing.

Bug: #30326
Change-Id: Ia61f540f1991ecdf42ccdb47b932890e636c1bd3
Reviewed-on: https://dart-review.googlesource.com/4181
Reviewed-by: Lasse R.H. Nielsen <lrn@google.com>
whesse pushed a commit that referenced this issue Sep 15, 2017
These are not supported in Dart 2.0:

#30326
Change-Id: I2e177a647c43af2bf7f37ff382234b12b8c2286c
Reviewed-on: https://dart-review.googlesource.com/4182
Reviewed-by: Jennifer Messerly <jmesserly@google.com>
@kevmoo
Copy link
Member

kevmoo commented Sep 19, 2017

What this probably means in practice is to start issuing a hint relatively soon indicating that this feature is obsolete (with a quick fix to add the parentheses). In 2.0 we can promote the hint to an error.

@bwilkerson Let's get on that sooner rather than later. I'm guessing that we can go back to assuming the type of the assert argument is bool?

Right, @munificent ?

kevmoo added a commit to angulardart/angular that referenced this issue Sep 19, 2017
DDC in Dart SDK 2.0.0-dev.0.0 removed this feature

More to come: dart-lang/sdk#30326
kevmoo added a commit to angulardart/angular that referenced this issue Sep 19, 2017
DDC in Dart SDK 2.0.0-dev.0.0 removed this feature

More to come: dart-lang/sdk#30326
kevmoo referenced this issue in dart-lang/linter Sep 19, 2017
alorenzen pushed a commit to angulardart/angular that referenced this issue Sep 20, 2017
DDC in Dart SDK 2.0.0-dev.0.0 removed this feature
Plans to remove it entirely in Dart 2: dart-lang/sdk#30326

Closes #634

PiperOrigin-RevId: 169286011
matanlurey pushed a commit to angulardart/angular that referenced this issue Sep 20, 2017
DDC in Dart SDK 2.0.0-dev.0.0 removed this feature
Plans to remove it entirely in Dart 2: dart-lang/sdk#30326

Closes #634

PiperOrigin-RevId: 169286011
@munificent
Copy link
Member Author

munificent commented Oct 3, 2017

I'm guessing that we can go back to assuming the type of the assert argument is bool?

We aren't going back to assuming the type is a bool. It's allowed, effectively, bool | (bool Function()) for ages. But, yes, we are going forward to only allowing bool.

nshahan pushed a commit to angulardart/angular_components that referenced this issue Jan 9, 2018
This is to prepare for the new language restriction: dart-lang/sdk#30326

PiperOrigin-RevId: 180941194
srawlins added a commit to srawlins/spritewidget that referenced this issue Jan 9, 2018
In Dart 2.0, only boolean values will be accepted by assert. Closures were previously accepted, but should simply self-evaluate now.

dart-lang/sdk#30326
alorenzen pushed a commit to angulardart/angular that referenced this issue Jan 12, 2018
This is to prepare for the new language restriction: dart-lang/sdk#30326

PiperOrigin-RevId: 181648657
alorenzen pushed a commit to angulardart/angular that referenced this issue Jan 16, 2018
This is to prepare for the new language restriction: dart-lang/sdk#30326

PiperOrigin-RevId: 181648657
alorenzen pushed a commit to angulardart/angular that referenced this issue Jan 17, 2018
This is to prepare for the new language restriction: dart-lang/sdk#30326

PiperOrigin-RevId: 181648657
alorenzen pushed a commit to angulardart/angular that referenced this issue Jan 17, 2018
This is to prepare for the new language restriction: dart-lang/sdk#30326

PiperOrigin-RevId: 181648657
alorenzen pushed a commit to angulardart/angular that referenced this issue Jan 17, 2018
This is to prepare for the new language restriction: dart-lang/sdk#30326

PiperOrigin-RevId: 181648657
nshahan pushed a commit to angulardart/angular_components that referenced this issue Jan 19, 2018
This is to prepare for the new language restriction: dart-lang/sdk#30326

PiperOrigin-RevId: 180941194
nshahan pushed a commit to angulardart/angular_components that referenced this issue Feb 6, 2018
In Dart 2.0, assert will only accept booleans, not closures any more. The solution to upgrade is very simple: we just self-execute any closures passed to assert today.

dart-lang/sdk#30326

PiperOrigin-RevId: 183011076
nshahan pushed a commit to angulardart/angular_components that referenced this issue Feb 7, 2018
In Dart 2.0, assert will only accept booleans, not closures any more. The solution to upgrade is very simple: we just self-execute any closures passed to assert today.

dart-lang/sdk#30326

PiperOrigin-RevId: 183011076
@srawlins
Copy link
Member

Support removed from analyzer in https://dart-review.googlesource.com/c/sdk/+/31963. All internal usage is fixed.

@srawlins
Copy link
Member

Also, I think some flutter testing this week revealed that closures-in-bool has been removed (was never written) in front-end.

@leafpetersen leafpetersen added this to Language implementation in 2.0 language Mar 3, 2018
@leafpetersen
Copy link
Member

Front end gives an error, as does the VM in preview-dart-2. The analyzer also gives an error now in strong mode. I believe that all of the code that we need to/plan to clean up is clean.

@dgrove
Copy link
Contributor

dgrove commented May 10, 2018

Is any work actually remaining here for Dart 2?

@srawlins
Copy link
Member

I guess that's just a question for dart2js, according to the checkboxes.

@sigmundch
Copy link
Member

dart2js is derived from the CFE in this case, so no action needed on our end. I just marked the checkbox.

@leafpetersen
Copy link
Member

I think that covers it.

2.0 language automation moved this from Language implementation to Done May 10, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-language Dart language related items (some items might be better tracked at github.com/dart-lang/language).
Projects
No open projects
2.0 language
  
Done
Development

No branches or pull requests

9 participants