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

Wrong order of evaluating map entries #56848

Open
sgrekhov opened this issue Oct 4, 2024 · 3 comments
Open

Wrong order of evaluating map entries #56848

sgrekhov opened this issue Oct 4, 2024 · 3 comments
Assignees
Labels
area-front-end Use area-front-end for front end / CFE / kernel format related issues. cfe-features General feature work in the CFE. feature-null-aware-elements Implementation of the Null-aware elements feature soundness type-bug Incorrect behavior (everything from a crash to more subtle misbehavior)

Comments

@sgrekhov
Copy link
Contributor

sgrekhov commented Oct 4, 2024

https://github.com/dart-lang/co19/blob/master/LanguageFeatures/Control-flow-collections/Null-aware-elements/runtime_A02_t02.dart fails at run time on VM, dart2js and dart2wasm.

String _log = "";

int? f(int? v) {
  _log += "f($v);";
  return v;
}

main() {
  var map1 = {
    f(1): ?f(2),
    ?f(3): ?f(null),
    ?f(4): ?null
  };
  Expect.mapEquals({1: 2}, map1);
  Expect.equals("f(1);f(2);f(3);f(null);f(4);", _log); // Expect.equals(expected: <f(1);f(2);f(3);f(null);f(4);>, actual: <f(2);f(1);f(3);f(null);f(4);>) fails.
...
}

Why map value is evaluated before the key? According to the spec the key should be evaluated first.

  • Else, if element is a nullAwareMapElement with entry k: v:

  • Evaluate k to a value kv.

  • If element has a null-aware key and kv is null, then stop. Else continue...

  • Evaluate v to a value vv.

  • If element has a null-aware value and vv is null, then stop. Else continue...

  • Append an entry kv: vv to result.

@sgrekhov sgrekhov added the feature-null-aware-elements Implementation of the Null-aware elements feature label Oct 4, 2024
@dart-github-bot
Copy link
Collaborator

Summary: The code in runtime_A02_t02.dart fails because the map values are evaluated before the keys, violating the specification which states that keys should be evaluated first. This discrepancy in evaluation order leads to incorrect map entries and a failed assertion.

@dart-github-bot dart-github-bot added area-language Dart language related items (some items might be better tracked at github.com/dart-lang/language). triage-automation See https://github.com/dart-lang/ecosystem/tree/main/pkgs/sdk_triage_bot. type-bug Incorrect behavior (everything from a crash to more subtle misbehavior) labels Oct 4, 2024
@lrhn
Copy link
Member

lrhn commented Oct 4, 2024

Nice optimization! Not sound, but nice thought. 😉

If ?f(2) would be null, the value of f(1) isn't needed, otherwise it's definitely needed, so we can just call it afterwards.
That only works if the functions have no side-effect. With side effects, and promotion from left to right, the key expression must be evaluated first, whether the value expression bails out or not.

Soundness example:

int? ifEven(int x) => x.isEven ? x : null; // x == null here!
void main() {
  int? x = null as dynamic;
  var map = {(x ??= 0) : ?ifEven(x)?.toRadixString(16)};
  print(map); // prints {} ?!?
}

which fails with

NoSuchMethodError: The getter 'isEven' was called on null.
Receiver: null
Tried calling: isEven
#0      Object.noSuchMethod (dart:core-patch/object_patch.dart:38:5)
#1      ifEven (file:///home/lrn/dart/sdk/dev/nae.dart:2:25)
...

which isn't sound.

Marking as front-end, since it affects multiple backends.

@lrhn lrhn added area-front-end Use area-front-end for front end / CFE / kernel format related issues. soundness and removed area-language Dart language related items (some items might be better tracked at github.com/dart-lang/language). triage-automation See https://github.com/dart-lang/ecosystem/tree/main/pkgs/sdk_triage_bot. labels Oct 4, 2024
@eernstg
Copy link
Member

eernstg commented Oct 4, 2024

Good catch!

@johnniwinther johnniwinther added the cfe-features General feature work in the CFE. label Oct 4, 2024
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-features General feature work in the CFE. feature-null-aware-elements Implementation of the Null-aware elements feature soundness type-bug Incorrect behavior (everything from a crash to more subtle misbehavior)
Projects
None yet
Development

No branches or pull requests

6 participants