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

dart fix for avoid_redundant_argument_values makes semantic changes when involving kIsWeb #47404

Closed
Hixie opened this issue Oct 7, 2021 · 4 comments
Assignees
Labels
area-analyzer Use area-analyzer for Dart analyzer issues, including the analysis server and code completion.

Comments

@Hixie
Copy link
Contributor

Hixie commented Oct 7, 2021

dart fix for avoid_redundant_argument_values makes semantic changes when involving kIsWeb.

STEPS TO REPRODUCE

  • dart create dart_fix_test
  • Replace the dart_fix_test/bin/dart_fix_test.dart file as follows:
void test({ bool foo = true }) { }

const bool kIsWeb = identical(0, 0.0);

void main(List<String> arguments) {
  test(foo: kIsWeb);
}
  • Replace the analysis_options.yaml file as follows:
linter:
  rules:
    - avoid_redundant_argument_values
  • Run dart fix --apply

EXPECTED RESULTS

The analyzer and dart fix should magically know that identical(0, 0.0) is not a constant value despite it being a constant according to the language, and thus make no change to the code above.

ACTUAL RESULTS

The argument is removed, because kIsWeb is assumed to be true (which is weird in itself, but that's another story) and so the argument is removed. This is, of course, a significant semantic change.

@bwilkerson
Copy link
Member

@scheglov @srawlins

This sounds like a possible issue with the constant evaluation engine. Should we consider identical(0, 0.0) to evaluate to a valid bool of unknown value? At one point I think we decided to optimize analyzer for the web case, but I'm not convinced that's still a valid approach (even assuming it was at the time).

@a-siva a-siva added the area-analyzer Use area-analyzer for Dart analyzer issues, including the analysis server and code completion. label Oct 14, 2021
@scheglov scheglov self-assigned this Oct 14, 2021
@scheglov
Copy link
Contributor

https://dart-review.googlesource.com/c/sdk/+/216923
Lets see what it will do to Flutter bots.

@scheglov
Copy link
Contributor

The original CL where kIsWeb workaround was added is https://dart-review.googlesource.com/c/sdk/+/135781.

copybara-service bot pushed a commit that referenced this issue Oct 14, 2021
…WN_VALUE

Bug: #47404
Change-Id: I29626bd11379580c6a090384c57882de98f619b1
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/216923
Commit-Queue: Konstantin Shcheglov <scheglov@google.com>
Reviewed-by: Samuel Rawlins <srawlins@google.com>
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
@scheglov
Copy link
Contributor

It looks fixed now with the CL landed.

dart_fix_test$ /Users/scheglov/Source/Dart/sdk.git/sdk/xcodebuild/ReleaseX64/dart-sdk/bin/dart fix --apply
Computing fixes in dart_fix_test... 3.6s
Nothing to fix!

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.
Projects
None yet
Development

No branches or pull requests

4 participants