[XL] Change in dart/sdk[main]: [cfe] Repair the evaluation order in null-aware map entries

0 views
Skip to first unread message

Chloe Stefantsova (Gerrit)

unread,
Oct 25, 2024, 5:00:21 AM10/25/24
to Chloe Stefantsova, Johnni Winther, Erik Ernst, Commit Queue, dart-fe-te...@google.com, rev...@dartlang.org
Attention needed from Erik Ernst and Johnni Winther

Chloe Stefantsova voted Commit-Queue+1

Commit-Queue+1
Open in Gerrit

Related details

Attention is currently required from:
  • Erik Ernst
  • Johnni Winther
Submit Requirements:
  • requirement is not satisfiedCode-Review
  • requirement is not satisfiedReview-Enforcement
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
Gerrit-MessageType: comment
Gerrit-Project: sdk
Gerrit-Branch: main
Gerrit-Change-Id: If1cda4a77cf42c0f75a6054c941b458cad13ebed
Gerrit-Change-Number: 391960
Gerrit-PatchSet: 3
Gerrit-Owner: Chloe Stefantsova <cstefa...@google.com>
Gerrit-Reviewer: Chloe Stefantsova <cstefa...@google.com>
Gerrit-Reviewer: Erik Ernst <eer...@google.com>
Gerrit-Reviewer: Johnni Winther <johnni...@google.com>
Gerrit-Attention: Johnni Winther <johnni...@google.com>
Gerrit-Attention: Erik Ernst <eer...@google.com>
Gerrit-Comment-Date: Fri, 25 Oct 2024 09:00:15 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
unsatisfied_requirement
open
diffy

Johnni Winther (Gerrit)

unread,
Oct 25, 2024, 7:30:05 AM10/25/24
to Chloe Stefantsova, Erik Ernst, Commit Queue, dart-fe-te...@google.com, rev...@dartlang.org
Attention needed from Chloe Stefantsova and Erik Ernst

Johnni Winther voted Code-Review+1

Code-Review+1
Open in Gerrit

Related details

Attention is currently required from:
  • Chloe Stefantsova
  • Erik Ernst
Submit Requirements:
  • requirement satisfiedCode-Review
  • requirement satisfiedReview-Enforcement
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
Gerrit-MessageType: comment
Gerrit-Project: sdk
Gerrit-Branch: main
Gerrit-Change-Id: If1cda4a77cf42c0f75a6054c941b458cad13ebed
Gerrit-Change-Number: 391960
Gerrit-PatchSet: 3
Gerrit-Owner: Chloe Stefantsova <cstefa...@google.com>
Gerrit-Reviewer: Chloe Stefantsova <cstefa...@google.com>
Gerrit-Reviewer: Erik Ernst <eer...@google.com>
Gerrit-Reviewer: Johnni Winther <johnni...@google.com>
Gerrit-Attention: Erik Ernst <eer...@google.com>
Gerrit-Attention: Chloe Stefantsova <cstefa...@google.com>
Gerrit-Comment-Date: Fri, 25 Oct 2024 11:30:00 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
satisfied_requirement
open
diffy

Erik Ernst (Gerrit)

unread,
Oct 25, 2024, 8:02:18 AM10/25/24
to Chloe Stefantsova, Johnni Winther, Commit Queue, dart-fe-te...@google.com, rev...@dartlang.org
Attention needed from Chloe Stefantsova

Erik Ernst voted and added 3 comments

Votes added by Erik Ernst

Code-Review+1

3 comments

Patchset-level comments
File-level comment, Patchset 3 (Latest):
Erik Ernst . resolved

LGTM, noting that it might be useful to add some test cases where the null-aware operator actually makes a difference.

File pkg/front_end/testcases/null_aware_elements/evaluation_order.dart
Line 12, Patchset 3 (Latest):main() {
Erik Ernst . unresolved

I don't know if this would add any actual coverage, but it seems natural to expect that `?e` will encounter null and omit the insertion of an element/pair into the given data structure. Currently it's all non-null.

File tests/language/null_aware_elements/evaluation_order_test.dart
Line 16, Patchset 3 (Latest):main() {
Erik Ernst . unresolved

Some consideration here: Perhaps add cases where `?e` has an `e` that evaluates to null?

Open in Gerrit

Related details

Attention is currently required from:
  • Chloe Stefantsova
Submit Requirements:
  • requirement satisfiedCode-Review
  • requirement satisfiedReview-Enforcement
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
Gerrit-MessageType: comment
Gerrit-Project: sdk
Gerrit-Branch: main
Gerrit-Change-Id: If1cda4a77cf42c0f75a6054c941b458cad13ebed
Gerrit-Change-Number: 391960
Gerrit-PatchSet: 3
Gerrit-Owner: Chloe Stefantsova <cstefa...@google.com>
Gerrit-Reviewer: Chloe Stefantsova <cstefa...@google.com>
Gerrit-Reviewer: Erik Ernst <eer...@google.com>
Gerrit-Reviewer: Johnni Winther <johnni...@google.com>
Gerrit-Attention: Chloe Stefantsova <cstefa...@google.com>
Gerrit-Comment-Date: Fri, 25 Oct 2024 12:02:11 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
satisfied_requirement
open
diffy

Chloe Stefantsova (Gerrit)

unread,
Oct 28, 2024, 3:37:41 AM10/28/24
to Chloe Stefantsova, Erik Ernst, Johnni Winther, Commit Queue, dart-fe-te...@google.com, rev...@dartlang.org

Chloe Stefantsova added 3 comments

Patchset-level comments
File-level comment, Patchset 4 (Latest):
Chloe Stefantsova . resolved

Thank you for taking a look and for the suggestions, Johnni and Erik!

File pkg/front_end/testcases/null_aware_elements/evaluation_order.dart
Line 12, Patchset 3:main() {
Erik Ernst . resolved

I don't know if this would add any actual coverage, but it seems natural to expect that `?e` will encounter null and omit the insertion of an element/pair into the given data structure. Currently it's all non-null.

Chloe Stefantsova

Thank you for the suggestion, Erik! I added test cases, where the expressions marked with `?` actually return `null`.

File tests/language/null_aware_elements/evaluation_order_test.dart
Line 16, Patchset 3:main() {
Erik Ernst . resolved

Some consideration here: Perhaps add cases where `?e` has an `e` that evaluates to null?

Chloe Stefantsova

I updated this test as well.

Open in Gerrit

Related details

Attention set is empty
Submit Requirements:
  • requirement satisfiedCode-Review
  • requirement satisfiedReview-Enforcement
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
Gerrit-MessageType: comment
Gerrit-Project: sdk
Gerrit-Branch: main
Gerrit-Change-Id: If1cda4a77cf42c0f75a6054c941b458cad13ebed
Gerrit-Change-Number: 391960
Gerrit-PatchSet: 4
Gerrit-Owner: Chloe Stefantsova <cstefa...@google.com>
Gerrit-Reviewer: Chloe Stefantsova <cstefa...@google.com>
Gerrit-Reviewer: Erik Ernst <eer...@google.com>
Gerrit-Reviewer: Johnni Winther <johnni...@google.com>
Gerrit-Comment-Date: Mon, 28 Oct 2024 07:37:37 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Erik Ernst <eer...@google.com>
satisfied_requirement
open
diffy

Chloe Stefantsova (Gerrit)

unread,
Oct 28, 2024, 3:37:50 AM10/28/24
to Chloe Stefantsova, Erik Ernst, Johnni Winther, Commit Queue, dart-fe-te...@google.com, rev...@dartlang.org

Chloe Stefantsova voted Commit-Queue+2

Commit-Queue+2
Open in Gerrit

Related details

Attention set is empty
Submit Requirements:
  • requirement satisfiedCode-Review
  • requirement satisfiedReview-Enforcement
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
Gerrit-MessageType: comment
Gerrit-Project: sdk
Gerrit-Branch: main
Gerrit-Change-Id: If1cda4a77cf42c0f75a6054c941b458cad13ebed
Gerrit-Change-Number: 391960
Gerrit-PatchSet: 4
Gerrit-Owner: Chloe Stefantsova <cstefa...@google.com>
Gerrit-Reviewer: Chloe Stefantsova <cstefa...@google.com>
Gerrit-Reviewer: Erik Ernst <eer...@google.com>
Gerrit-Reviewer: Johnni Winther <johnni...@google.com>
Gerrit-Comment-Date: Mon, 28 Oct 2024 07:37:45 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
satisfied_requirement
open
diffy

Chloe Stefantsova (Gerrit)

unread,
Oct 28, 2024, 3:57:25 AM10/28/24
to Chloe Stefantsova, Erik Ernst, Johnni Winther, Commit Queue, dart-fe-te...@google.com, rev...@dartlang.org

Chloe Stefantsova voted Commit-Queue+2

Commit-Queue+2
Open in Gerrit

Related details

Attention set is empty
Submit Requirements:
  • requirement satisfiedCode-Review
  • requirement satisfiedReview-Enforcement
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
Gerrit-MessageType: comment
Gerrit-Project: sdk
Gerrit-Branch: main
Gerrit-Change-Id: If1cda4a77cf42c0f75a6054c941b458cad13ebed
Gerrit-Change-Number: 391960
Gerrit-PatchSet: 5
Gerrit-Owner: Chloe Stefantsova <cstefa...@google.com>
Gerrit-Reviewer: Chloe Stefantsova <cstefa...@google.com>
Gerrit-Reviewer: Erik Ernst <eer...@google.com>
Gerrit-Reviewer: Johnni Winther <johnni...@google.com>
Gerrit-Comment-Date: Mon, 28 Oct 2024 07:57:21 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
satisfied_requirement
open
diffy

Commit Queue (Gerrit)

unread,
Oct 28, 2024, 4:33:51 AM10/28/24
to Chloe Stefantsova, Erik Ernst, Johnni Winther, dart-fe-te...@google.com, rev...@dartlang.org

Commit Queue submitted the change with unreviewed changes

Unreviewed changes

3 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:

```
The name of the file: pkg/front_end/testcases/null_aware_elements/evaluation_order.dart.strong.expect
Insertions: 103, Deletions: 42.

The diff is too large to show. Please review the diff.
```
```
The name of the file: pkg/front_end/testcases/null_aware_elements/evaluation_order.dart.strong.transformed.expect
Insertions: 103, Deletions: 42.

The diff is too large to show. Please review the diff.
```
```
The name of the file: tests/language/null_aware_elements/evaluation_order_test.dart
Insertions: 26, Deletions: 3.

The diff is too large to show. Please review the diff.
```
```
The name of the file: pkg/front_end/testcases/null_aware_elements/evaluation_order.dart
Insertions: 26, Deletions: 3.

The diff is too large to show. Please review the diff.
```
```
The name of the file: pkg/front_end/testcases/null_aware_elements/evaluation_order.dart.strong.modular.expect
Insertions: 103, Deletions: 42.

The diff is too large to show. Please review the diff.
```

Change information

Commit message:
[cfe] Repair the evaluation order in null-aware map entries

Closes https://github.com/dart-lang/sdk/issues/56848
Part of https://github.com/dart-lang/sdk/issues/55955
Change-Id: If1cda4a77cf42c0f75a6054c941b458cad13ebed
Commit-Queue: Chloe Stefantsova <cstefa...@google.com>
Reviewed-by: Johnni Winther <johnni...@google.com>
Reviewed-by: Erik Ernst <eer...@google.com>
Files:
  • M pkg/front_end/lib/src/type_inference/inference_visitor.dart
  • A pkg/front_end/testcases/null_aware_elements/evaluation_order.dart
  • A pkg/front_end/testcases/null_aware_elements/evaluation_order.dart.strong.expect
  • A pkg/front_end/testcases/null_aware_elements/evaluation_order.dart.strong.modular.expect
  • A pkg/front_end/testcases/null_aware_elements/evaluation_order.dart.strong.outline.expect
  • A pkg/front_end/testcases/null_aware_elements/evaluation_order.dart.strong.transformed.expect
  • A pkg/front_end/testcases/null_aware_elements/evaluation_order.dart.textual_outline.expect
  • A pkg/front_end/testcases/null_aware_elements/evaluation_order.dart.textual_outline_modelled.expect
  • M pkg/front_end/testcases/null_aware_elements/inference.dart.strong.expect
  • M pkg/front_end/testcases/null_aware_elements/inference.dart.strong.modular.expect
  • M pkg/front_end/testcases/null_aware_elements/inference.dart.strong.transformed.expect
  • M pkg/front_end/testcases/null_aware_elements/map_type_inference_correction.dart.strong.expect
  • M pkg/front_end/testcases/null_aware_elements/map_type_inference_correction.dart.strong.modular.expect
  • M pkg/front_end/testcases/null_aware_elements/map_type_inference_correction.dart.strong.transformed.expect
  • M pkg/front_end/testcases/null_aware_elements/simple_positive.dart.strong.expect
  • M pkg/front_end/testcases/null_aware_elements/simple_positive.dart.strong.modular.expect
  • M pkg/front_end/testcases/null_aware_elements/simple_positive.dart.strong.transformed.expect
  • M pkg/front_end/testcases/null_aware_elements/type_inference_simple_positive.dart.strong.expect
  • M pkg/front_end/testcases/null_aware_elements/type_inference_simple_positive.dart.strong.modular.expect
  • M pkg/front_end/testcases/null_aware_elements/type_inference_simple_positive.dart.strong.transformed.expect
  • A tests/language/null_aware_elements/evaluation_order_test.dart
Change size: XL
Delta: 21 files changed, 1390 insertions(+), 660 deletions(-)
Branch: refs/heads/main
Submit Requirements:
  • requirement satisfiedCode-Review: +1 by Johnni Winther, +1 by Erik Ernst
Open in Gerrit
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
Gerrit-MessageType: merged
Gerrit-Project: sdk
Gerrit-Branch: main
Gerrit-Change-Id: If1cda4a77cf42c0f75a6054c941b458cad13ebed
Gerrit-Change-Number: 391960
Gerrit-PatchSet: 6
Gerrit-Owner: Chloe Stefantsova <cstefa...@google.com>
Gerrit-Reviewer: Chloe Stefantsova <cstefa...@google.com>
open
diffy
satisfied_requirement
Reply all
Reply to author
Forward
0 new messages