From c007d3bc0cb4b1e2b849005088329bbef5b3bf28 Mon Sep 17 00:00:00 2001 From: Paul Berry Date: Thu, 6 Jun 2024 19:15:57 +0000 Subject: [PATCH] Front end: type infer all record literal fields before hoisting. Previously, if a record literal had named fields, type inference of the literal fields would happen as part of the process of hoisting. This was a problem because the hoisting process requires visiting the fields in reverse order, so as a result, the order in which subexpressions were type inferred was the reverse of the order in which they would be executed, leading to unsound type promotion behavior. Fixes #55914. Bug: https://github.com/dart-lang/sdk/issues/55914 Change-Id: I2d8930e0e1d7579d065bfb850aa7472d28a8012c Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/369761 Reviewed-by: Chloe Stefantsova Commit-Queue: Paul Berry --- .../type_inference/inference_visitor.dart | 94 +++++++------ ...type_inference_named_field_order_test.dart | 130 ++++++++++++++++++ 2 files changed, 182 insertions(+), 42 deletions(-) create mode 100644 tests/language/records/type_inference_named_field_order_test.dart diff --git a/pkg/front_end/lib/src/fasta/type_inference/inference_visitor.dart b/pkg/front_end/lib/src/fasta/type_inference/inference_visitor.dart index 39389c6eaba5..7c1a3fa17da5 100644 --- a/pkg/front_end/lib/src/fasta/type_inference/inference_visitor.dart +++ b/pkg/front_end/lib/src/fasta/type_inference/inference_visitor.dart @@ -9152,20 +9152,58 @@ class InferenceVisitorImpl extends InferenceVisitorBase positional[index] = expressionResult.expression; } } else { - List sortedNames = namedElements.keys.toList()..sort(); - positionalTypes = new List.filled(positional.length, const UnknownType()); Map namedElementTypes = {}; + // Index into [positional] of the positional element we find next. + int positionalIndex = 0; + + for (int index = 0; index < originalElementOrder.length; index++) { + Object element = originalElementOrder[index]; + if (element is NamedExpression) { + DartType contextType = + namedTypeContexts?[element.name] ?? const UnknownType(); + ExpressionInferenceResult expressionResult = + inferExpression(element.value, contextType); + if (contextType is! UnknownType) { + expressionResult = coerceExpressionForAssignment( + contextType, expressionResult, + treeNodeForTesting: node) ?? + expressionResult; + } + Expression expression = expressionResult.expression; + DartType type = expressionResult.postCoercionType ?? + expressionResult.inferredType; + element.value = expression..parent = element; + namedElementTypes[element.name] = type; + } else { + DartType contextType = + positionalTypeContexts?[positionalIndex] ?? const UnknownType(); + ExpressionInferenceResult expressionResult = + inferExpression(element as Expression, contextType); + if (contextType is! UnknownType) { + expressionResult = coerceExpressionForAssignment( + contextType, expressionResult, + treeNodeForTesting: node) ?? + expressionResult; + } + Expression expression = expressionResult.expression; + DartType type = expressionResult.postCoercionType ?? + expressionResult.inferredType; + positional[positionalIndex] = expression; + positionalTypes[positionalIndex] = type; + positionalIndex++; + } + } + + List sortedNames = namedElements.keys.toList()..sort(); + // Index into [sortedNames] of the named element we expected to find // next, for the named elements to be sorted. This also used to detect // when all named elements have been seen, even when they are not sorted. int nameIndex = sortedNames.length - 1; - // Index into [positional] of the positional element we find next. - int positionalIndex = positional.length - 1; - // For const literals we don't hoist to avoid using let variables in // inside constants. Since the elements of the literal must be constant // themselves, we know that there is no side effects of performing @@ -9183,22 +9221,12 @@ class InferenceVisitorImpl extends InferenceVisitorBase // expressions we need to hoist. When we observe an element out of order, // either positional after named or unsorted named, all preceding // elements must be hoisted to retain the original evaluation order. + positionalIndex--; for (int index = originalElementOrder.length - 1; index >= 0; index--) { Object element = originalElementOrder[index]; if (element is NamedExpression) { - DartType contextType = - namedTypeContexts?[element.name] ?? const UnknownType(); - ExpressionInferenceResult expressionResult = - inferExpression(element.value, contextType); - if (contextType is! UnknownType) { - expressionResult = coerceExpressionForAssignment( - contextType, expressionResult, - treeNodeForTesting: node) ?? - expressionResult; - } - Expression expression = expressionResult.expression; - DartType type = expressionResult.postCoercionType ?? - expressionResult.inferredType; + Expression expression = element.value; + DartType type = namedElementTypes[element.name]!; // TODO(johnniwinther): Should we use [isPureExpression] as is, make // it include (simple) literals, or add a new predicate? if (needsHoisting && !isPureExpression(expression)) { @@ -9208,10 +9236,7 @@ class InferenceVisitorImpl extends InferenceVisitorBase hoistedExpressions ??= []; hoistedExpressions.add(variable); element.value = createVariableGet(variable)..parent = element; - } else { - element.value = expression..parent = element; } - namedElementTypes[element.name] = type; if (!namedNeedsSorting && element.name != sortedNames[nameIndex]) { // Named elements are not sorted, so we need to hoist and sort them. namedNeedsSorting = true; @@ -9219,19 +9244,8 @@ class InferenceVisitorImpl extends InferenceVisitorBase } nameIndex--; } else { - DartType contextType = - positionalTypeContexts?[positionalIndex] ?? const UnknownType(); - ExpressionInferenceResult expressionResult = - inferExpression(element as Expression, contextType); - if (contextType is! UnknownType) { - expressionResult = coerceExpressionForAssignment( - contextType, expressionResult, - treeNodeForTesting: node) ?? - expressionResult; - } - Expression expression = expressionResult.expression; - DartType type = expressionResult.postCoercionType ?? - expressionResult.inferredType; + Expression expression = positional[positionalIndex]; + DartType type = positionalTypes[positionalIndex]; // TODO(johnniwinther): Should we use [isPureExpression] as is, make // it include (simple) literals, or add a new predicate? if (needsHoisting && !isPureExpression(expression)) { @@ -9241,15 +9255,11 @@ class InferenceVisitorImpl extends InferenceVisitorBase hoistedExpressions ??= []; hoistedExpressions.add(variable); positional[positionalIndex] = createVariableGet(variable); - } else { - positional[positionalIndex] = expression; - if (nameIndex >= 0) { - // We have not seen all named elements yet, so we must hoist the - // remaining named elements and the preceding positional elements. - needsHoisting = enableHoisting; - } + } else if (nameIndex >= 0) { + // We have not seen all named elements yet, so we must hoist the + // remaining named elements and the preceding positional elements. + needsHoisting = enableHoisting; } - positionalTypes[positionalIndex] = type; positionalIndex--; } } diff --git a/tests/language/records/type_inference_named_field_order_test.dart b/tests/language/records/type_inference_named_field_order_test.dart new file mode 100644 index 000000000000..e4a72aaec7f6 --- /dev/null +++ b/tests/language/records/type_inference_named_field_order_test.dart @@ -0,0 +1,130 @@ +// Copyright (c) 2024, the Dart project authors. Please see the AUTHORS file +// for details. All rights reserved. Use of this source code is governed by a +// BSD-style license that can be found in the LICENSE file. + +/// Test that if a record literal contains named fields, then all of the record +/// literal's fields are type inferred in the order in which they are written, +/// regardless of the sort order of the field names, and regardless of the +/// relative ordering of named and unnamed fields. +/// +/// See https://github.com/dart-lang/sdk/issues/55914 + +import '../static_type_helper.dart'; + +({Object a, Object b, Object c}) testSortedNamedFields(Object o1, Object o2) { + // Check that `a` is type inferred before `b` by promoting `o1` in `a` and + // checking its type in `b`. Check that `b` is type inferred before `c` by + // promoting `o2` in `b` and checking its type in `c`. + return ( + a: o1 as int, + b: [o1..expectStaticType>(), o2 as int], + c: o2..expectStaticType>() + ); +} + +({Object a, Object b, Object c}) testReversedNamedFields(Object o1, Object o2) { + // Check that `c` is type inferred before `b` by promoting `o1` in `c` and + // checking its type in `b`. Check that `b` is type inferred before `a` by + // promoting `o2` in `b` and checking its type in `a`. + return ( + c: o1 as int, + b: [o1..expectStaticType>(), o2 as int], + a: o2..expectStaticType>() + ); +} + +(Object, {Object a, Object b}) testSortedNamedFieldsAfterPositional( + Object o1, Object o2) { + // Check that `$1` is type inferred before `a` by promoting `o1` in `$1` and + // checking its type in `a`. Check that `a` is type inferred before `b` by + // promoting `o2` in `a` and checking its type in `b`. + return ( + o1 as int, + a: [o1..expectStaticType>(), o2 as int], + b: o2..expectStaticType>() + ); +} + +(Object, {Object a, Object b}) testReversedNamedFieldsAfterPositional( + Object o1, Object o2) { + // Check that `$1` is type inferred before `b` by promoting `o1` in `$1` and + // checking its type in `b`. Check that `b` is type inferred before `a` by + // promoting `o2` in `b` and checking its type in `a`. + return ( + o1 as int, + b: [o1..expectStaticType>(), o2 as int], + a: o2..expectStaticType>() + ); +} + +(Object, {Object a, Object b}) testSortedNamedFieldsBeforePositional( + Object o1, Object o2) { + // Check that `a` is type inferred before `b` by promoting `o1` in `a` and + // checking its type in `b`. Check that `b` is type inferred before `$1` by + // promoting `o2` in `b` and checking its type in `$1`. + return ( + a: o1 as int, + b: [o1..expectStaticType>(), o2 as int], + o2..expectStaticType>() + ); +} + +(Object, {Object a, Object b}) testReversedNamedFieldsBeforePositional( + Object o1, Object o2) { + // Check that `b` is type inferred before `a` by promoting `o1` in `b` and + // checking its type in `a`. Check that `a` is type inferred before `$1` by + // promoting `o2` in `a` and checking its type in `$1`. + return ( + b: o1 as int, + a: [o1..expectStaticType>(), o2 as int], + o2..expectStaticType>() + ); +} + +(Object, Object, {Object a}) testSingleNamedFieldAfterPositionals( + Object o1, Object o2) { + // Check that `$1` is type inferred before `$2` by promoting `o1` in `$1` and + // checking its type in `$2`. Check that `$2` is type inferred before `a` by + // promoting `o2` in `$2` and checking its type in `a`. + return ( + o1 as int, + [o1..expectStaticType>(), o2 as int], + a: o2..expectStaticType>() + ); +} + +(Object, Object, {Object a}) testSingleNamedFieldBetweenPositionals( + Object o1, Object o2) { + // Check that `$1` is type inferred before `a` by promoting `o1` in `$1` and + // checking its type in `a`. Check that `a` is type inferred before `$2` by + // promoting `o2` in `a` and checking its type in `$2`. + return ( + o1 as int, + a: [o1..expectStaticType>(), o2 as int], + o2..expectStaticType>() + ); +} + +(Object, Object, {Object a}) testSingleNamedFieldBeforePositionals( + Object o1, Object o2) { + // Check that `a` is type inferred before `$1` by promoting `o1` in `a` and + // checking its type in `$1`. Check that `$1` is type inferred before `$2` by + // promoting `o2` in `$1` and checking its type in `$2`. + return ( + a: o1 as int, + [o1..expectStaticType>(), o2 as int], + o2..expectStaticType>() + ); +} + +main() { + testSortedNamedFields(0, 1); + testReversedNamedFields(0, 1); + testSortedNamedFieldsAfterPositional(0, 1); + testReversedNamedFieldsAfterPositional(0, 1); + testSortedNamedFieldsBeforePositional(0, 1); + testReversedNamedFieldsBeforePositional(0, 1); + testSingleNamedFieldAfterPositionals(0, 1); + testSingleNamedFieldBetweenPositionals(0, 1); + testSingleNamedFieldBeforePositionals(0, 1); +}