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

Analyser reports no issues for erroring dart program [records] #55914

Closed
treeplate opened this issue Jun 3, 2024 · 3 comments
Closed

Analyser reports no issues for erroring dart program [records] #55914

treeplate opened this issue Jun 3, 2024 · 3 comments
Assignees
Labels
area-front-end Use area-front-end for front end / CFE / kernel format related issues. cfe-type-inference CFE type inference issues soundness

Comments

@treeplate
Copy link

void foo(final Object? e) {
  (
    user: (e as Map<String, Object?>),
    message: e['text'],
  );
}

Analyser reports no issues.
dart run:

test.dart:4:15: Error: The operator '[]' isn't defined for the class 'Object?'.
 - 'Object' is from 'dart:core'.
Try correcting the operator to an existing operator, or defining a '[]' operator.
    message: e['text'],
              ^

dart info:


If providing this information as part of reporting a bug, please review the information
below to ensure it only contains things you're comfortable posting publicly.

#### General info

- Dart 3.5.0-214.0.dev (dev) (Fri May 31 05:06:48 2024 -0700) on "macos_arm64"
- on macos / Version 14.4.1 (Build 23E224)
- locale is en-US

#### Project info

- sdk constraint: '^3.5.0-191.0.dev'
- dependencies: http
- dev_dependencies: lints, test

#### Process info

| Memory |   CPU | Elapsed time | Command line                                                                    |
| -----: | ----: | -----------: | ------------------------------------------------------------------------------- |
|  40 MB |  0.0% |  29-21:52:30 | dart devtools --no-launch-browser                                               |
|  55 MB |  0.0% |  24-14:14:37 | dart devtools --no-launch-browser                                               |
|  51 MB |  0.0% |  24-14:12:55 | dart devtools --no-launch-browser                                               |
|  43 MB |  0.0% |  24-14:08:56 | dart devtools --no-launch-browser                                               |
|  43 MB |  0.0% |  24-14:08:35 | dart devtools --no-launch-browser                                               |
|  43 MB |  0.0% |  24-14:08:26 | dart devtools --no-launch-browser                                               |
| 433 MB | 36.7% |        01:42 | dart language-server --protocol=lsp --client-id=VS-Code --client-version=3.90.0 |
|  82 MB |  0.0% |        01:42 | dart tooling-daemon --machine                                                   |
@parlough parlough added the area-front-end Use area-front-end for front end / CFE / kernel format related issues. label Jun 3, 2024
@parlough
Copy link
Member

parlough commented Jun 3, 2024

Thanks for the issue!

I'm going to tentatively triage this as a compiler frontend issue, as it appears the analyzer is correct if type promotion occurs with the cast.

\cc @munificent @stereotype441 Should type promotion occur due to the evaluation order of the record fields like the analyzer behaves here?

@stereotype441 stereotype441 added soundness cfe-type-inference CFE type inference issues labels Jun 4, 2024
@stereotype441 stereotype441 self-assigned this Jun 4, 2024
@stereotype441
Copy link
Member

\cc @munificent @stereotype441 Should type promotion occur due to the evaluation order of the record fields like the analyzer behaves here?

@parlough
Yes, it absolutely should! This is definitely a bug in the compiler front end.

I dug into the compiler logic, and it seems that what happens is that when a record literal contains at least one named field, that triggers some special logic in the front end that figures out whether it will need to introduce temporary internal variables to ensure that when named fields are sorted, it doesn't affect evaluation order (e.g. (b: foo(), a: bar()) gets rewritten to let x = foo() in (a: bar(), b: x).

Doing that rewrite is fine. The problem is, in order to figure out whether the rewrite is necessary (and how many temporary internal variables to introduce), the front end uses a clever loop that traverses the record fields in reverse order. Since that loop is the loop that triggers type inference for the subexpressions, it means the subexpressions are being type inferred in a different order than the order in which they will ultimately be executed. Not only does that mess up type promotion in this example, but it could in principle result in unsound behavior. For example, the following code is accepted by the front end but causes a call to null when run:

int f(int Function()? g) {
  var x = (b: g(), a: g!);
  return x.b;
}
main() {
  f(null);
}

Whereas the analyzer correctly rejects this code.

I think I can do a fairly simple fix (just type infer all the fields, in order, before the loop that introduces temporary variables). I'll try to do that today; if I can't get it to work, I'll turn it over to the front end team.

@stereotype441
Copy link
Member

Fix is out for review: https://dart-review.googlesource.com/c/sdk/+/369761

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-front-end Use area-front-end for front end / CFE / kernel format related issues. cfe-type-inference CFE type inference issues soundness
Projects
None yet
Development

No branches or pull requests

3 participants