dart-lang / sdk

The Dart SDK, including the VM, JS and Wasm compilers, analysis, core libraries, and more.
https://dart.dev
BSD 3-Clause "New" or "Revised" License
10.21k stars 1.57k forks source link

Possible to create a const constructor that then cannot be used in a const context #47273

Closed wujek-srujek closed 1 year ago

wujek-srujek commented 3 years ago

I have a case of a class which has a const constructor that is then not callable in a const context:

import 'package:meta/meta.dart';

@immutable
class Sample {
  final List<String> strings;

  const Sample({
    required this.strings,
  })  : assert(strings.length >= 1);
}

The compiler complains with 'Evaluation of this constant expression throws an exception.' when I write this:

const s = Sample(strings: ['abc']);

If I make the constructor not const, my linter complains that @immutable classes should have a const constructor. I know that I can ignore it, but the question is - why does the compiler allow writing code which cannot be called? I would expect the analyzer/compiler to fail earlier, right where the constructor is defined.

stevemessick commented 3 years ago

@lrhn @munificent @leafpetersen – should this be moved to the language repo?

leafpetersen commented 3 years ago

This is working correctly and as specified.

An expression of the form e.length is potentially constant if e is a potentially constant expression. It is further constant if e is a constant expression that evaluates to an instance of String, such that length denotes
an instance getter invocation.

strings.length is a potentially constant expression, so the declaration of the const constructor is valid, but the invocation is invalid since strings does not evaluate to a String. Agreed that this is a less than useful specification, since strings can never evaluate to something of type String. @lrhn @eernstg I thought that at some point we talked about fixing this specification so that .length would only be potentially constant if the target was dynamic or String?

Modulo changes to the spec, I see two possible actionable things here in the short term:

1) This seems like a great candidate for an analyzer warning (and there may be a few other things in the this category). cc @bwilkerson @scheglov @srawlins

2) The analyzer error message is exceptionally unhelpful here. The CFE error is much better (see below). cc @bwilkerson @stereotype441 @srawlins who have talked about trying to push on error message quality/consistency.

leafp-macbookpro:Constructor-tear-offs leafp$ ~/src/dart-repo/sdk/xcodebuild/ReleaseX64/dart-sdk/bin/dart  ~/tmp/test.dart
../../../../../../../../tmp/test.dart:10:9: Error: Constant evaluation error:
  const Sample(strings : ["abc"]);
        ^
../../../../../../../../tmp/test.dart:6:24: Context: The property 'length' can't be accessed on '<String>["abc"]' in a constant expression.
  })  : assert(strings.length >= 1);
                       ^
leafp-macbookpro:Constructor-tear-offs leafp$ ~/src/dart-repo/sdk/xcodebuild/ReleaseX64/dart-sdk/bin/dartanalyzer  ~/tmp/test.dart
Analyzing /Users/leafp/tmp/test.dart...
  error • Evaluation of this constant expression throws an exception. • /Users/leafp/tmp/test.dart:10:3 • const_eval_throws_exception
1 error found.
stereotype441 commented 3 years ago
  1. The analyzer error message is exceptionally unhelpful here. The CFE error is much better (see below). cc @bwilkerson @stereotype441 @srawlins who have talked about trying to push on error message quality/consistency.

Yeah, the analyzer's constant evaluator is really bad at error reporting. Since constant evaluation is essentially a limited form of code execution, it would make sense to just display a stack trace when something like this happens.

leafpetersen commented 3 years ago

Yeah, the analyzer's constant evaluator is really bad at error reporting. Since constant evaluation is essentially a limited form of code execution, it would make sense to just display a stack trace when something like this happens.

Possibly, but I don't think that's enough. I think that would be reasonable if this was an assertion failure, but here, it's that the code in the assertion is not a valid constant.

stereotype441 commented 3 years ago

Yeah, the analyzer's constant evaluator is really bad at error reporting. Since constant evaluation is essentially a limited form of code execution, it would make sense to just display a stack trace when something like this happens.

Possibly, but I don't think that's enough. I think that would be reasonable if this was an assertion failure, but here, it's that the code in the assertion is not a valid constant.

Yeah, the kind of thing I was imagining would look something like:

test.dart:9:24: Error: during constant evaluation, `.length` can only be used on strings, not on `List<String>`.
  })  : assert(strings.length >= 1);
                       ^^^^^^
test.dart:12:10: Context: while evaluating constant
const s = Sample(strings: ['abc']);
          ^^^^^^

(With additional context lines if the constant evaluation stack goes deeper than this)

lrhn commented 3 years ago

We originally had rules saying that c.length would only be potentially constant if c had type String or dynamic, but we took the type checking out of being "potentially constant" due to requests from the parser writers at the time. They wanted to be able to check during parsing whether something was potentially constant. That means that the language specification doesn't prevent you from writing something which we know will fail due to type-checking during constant evaluation (and because of that, it's technically a valid program as long as you never call the constructor as const).

I think that was a mistake, and we should fix it at some point. It's technically breaking, but not something I'd worry about - any const constructor broken by the change is never called as const anyway, so you can just remove the const from the constructor.

We can define being potentially constant as a predicate that can be tested (and failed) at any stage of compilation, and just let the parser allow any expression in const constructors, with the side-requirement that they must be potentially constant, which we then check later. (That is, it doesn't have to be a parsing error to write const Foo() : x = print("hello world");. As long as it's eventually a compile-time error, everything is fine.)