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

Analyzer: "assert(list.length > 0)" does not produce an error despite it being one #52833

Closed
matanlurey opened this issue Jul 3, 2023 · 3 comments
Labels
analyzer-constants analyzer-ux area-analyzer Use area-analyzer for Dart analyzer issues, including the analysis server and code completion. P2 A bug or feature request we're likely to work on

Comments

@matanlurey
Copy link
Contributor

This seems like a regression, but I couldn't tell you since when/what version.

$ dart --version
Dart SDK Version: 3.0.5 (stable) (Mon Jun 12 18:31:49 2023 +0000) on "macos_arm64"
void main() {
  const RequiresNonEmptyList([1]);
}

class RequiresNonEmptyList {
  const RequiresNonEmptyList(List<int> numbers) : assert(numbers.length > 0);
}

This cause a constant evaluation error at the call-site const RequiresNonEmptyList([1]), but the actual error is never shown by the analyzer - I have to use another tool (such as Dart2JS) to reveal the root cause:

Error compiling to JavaScript:
Info: Compiling with sound null safety.
lib/main.dart:2:9:
Error: Constant evaluation error:
  const RequiresNonEmptyList([]);
        ^
lib/main.dart:6:66:
Info: The property 'length' can't be accessed on '<int>[]' in a constant expression.
  const RequiresNonEmptyList(List<int> numbers) : assert(numbers.length > 0);

I'm guessing there was plans to allow constant evaluations of <List>.length but it was never fully implemented or requires some sort of flag/experiments that aren't typically enabled - in either case this is confusing behavior to (statically) allow something that is invalid.

For example, if I write : assert(numbers.isNotEmpty), I get (in the analyzer):

Invalid constant value.

(Which isn't a great error message, but at least it's shown within the assert)

@lrhn lrhn added the area-analyzer Use area-analyzer for Dart analyzer issues, including the analysis server and code completion. label Jul 3, 2023
@scheglov scheglov added P2 A bug or feature request we're likely to work on analyzer-spec Issues with the analyzer's implementation of the language spec analyzer-ux and removed analyzer-spec Issues with the analyzer's implementation of the language spec labels Jul 3, 2023
@scheglov
Copy link
Contributor

scheglov commented Jul 3, 2023

@kallentu

@lrhn
Copy link
Member

lrhn commented Jul 3, 2023

The plan was to allow String.length as a constant expression, and to allow the parser to recognize compile-time constant expressions without understanding types. That's why e.length is a compile time constant expression if e is one, and a potentially constant expression if e is one, independently of types.

If that potentially constant expression is actually used as a constant (constructor is invoked as const), then e must be a String. Otherwise there is no type requirement.

It's not great, and I'd like to change it at some point, so we reject a potentially constant expression of the form e.length when the static type of e is not assignable to String. But that's a breaking change, and a low priority, so nothing has happened.

The program does have a compile-time error, because [1].length is evaluated as a constant, and the analyzer should report that.

copybara-service bot pushed a commit that referenced this issue Aug 11, 2023
…expressions.

Add a new error message `CONST_EVAL_PROPERTY_ACCESS` for when we try to get 'length' on anything that's not a `String`.

This adds more specific errors for string length that would've went to the catch-all error at the end of `_getConstantValue` before. It should be more clear now what the error is.

Fixes #47273, #52833

Bug: #52833, #47273
Change-Id: I1f5daed82be00c6a8ecd43384d7ff9e59219cbb4
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/319160
Commit-Queue: Kallen Tu <kallentu@google.com>
Reviewed-by: Sigmund Cherem <sigmund@google.com>
Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
@matanlurey
Copy link
Contributor Author

Wow, thank you Kallen!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
analyzer-constants analyzer-ux area-analyzer Use area-analyzer for Dart analyzer issues, including the analysis server and code completion. P2 A bug or feature request we're likely to work on
Projects
None yet
Development

No branches or pull requests

4 participants