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.12k stars 1.57k forks source link

Possible bug in diagnostics for `mustBeConst` #55573

Open bwilkerson opened 5 months ago

bwilkerson commented 5 months ago

I would expect the following code to be valid, but it generated a diagnostic claiming that the argument to g isn't a constant expression, even though it is a constant expression:

import 'package:meta/meta.dart';

void f() {
  g(const C());
}

void g(@mustBeConst C c) {}

class C {
  const C();
}

Is that behavior intentional? Is it intended that only uses of const variables is allowed?

bwilkerson commented 5 months ago

@mosuem

mosuem commented 5 months ago

I don't think that's intentional, let me check.

dnys1 commented 4 days ago

The linter doesn't seem to consider const expressions as valid, only const variable references:

import 'package:meta/meta.dart';

class A {
  A(@mustBeConst this.value);

  final B value;
}

class B {
  const B();
}

const value = B();
final valid = A(value);
final invalid = A(const B());
Screenshot 2024-09-30 at 9 16 16 AM
mosuem commented 3 days ago

The code for recognizing constness uses the ConstantEvaluator, which only resolves primitives. Looking at the tests, it seems to operate on the parsed non-resolved AST, meaning it cannot handle things like SimpleIdentifiers etc.

Is there another ConstantEvaluator which I could use? This use case doesn't even need the actual value, just to know if it is const or not. The current solution of the ConstantEvaluator + some manual patch code seems to be less than ideal.

Any ideas @bwilkerson @srawlins @scheglov ?

bwilkerson commented 3 days ago

Would the method Expression.inConstantContext satisfy the requirement?

mosuem commented 3 days ago

I think it does not - 16/40 test cases fail when relying solely on Expression.inConstantContext.

bwilkerson commented 3 days ago

Based on a quick glance at the tests, and having no idea which tests are failing (so not a very thorough investigation), I'm guessing that the problem is references to simple identifiers. The reference doesn't have to be in a constant context, it just has to be a reference to a constant variable. For that, all you need to do is get the element, ensure that it's a variable, and ask whether the variable was declared as being const.

(Is the algorithm that you're trying to implemented documented somewhere? I might be able to give better answers if I understood the problem better.)

mosuem commented 2 days ago

That sounds good - I will try to implement a custom set of rules to catch the cases as good as possible. I filed https://dart-review.googlesource.com/c/sdk/+/387721.

Is the algorithm that you're trying to implemented documented somewhere?

It is not - the idea is just to recognize const parameters as well as possible, with the idea that those arguments can then possibly be recorded at compile time.