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.04k stars 1.55k forks source link

language/string/interpolation_and_buffer_test fails #53981

Open a-siva opened 9 months ago

a-siva commented 9 months ago
DART_CONFIGURATION=ReleaseARM64 TEST_COMPILATION_DIR=/b/s/w/ir/cache/builder/sdk/out/ReleaseARM64/generated_compilations/vm-aot-linux-release-arm64/tests_language_string_interpolation_and_buffer_test out/ReleaseARM64/dart_precompiled_runtime --sound-null-safety -Dtest_runner.configuration=vm-aot-linux-release-arm64 --ignore-unrecognized-flags --packages=/b/s/w/ir/cache/builder/sdk/.dart_tool/package_config.json /b/s/w/ir/cache/builder/sdk/out/ReleaseARM64/generated_compilations/vm-aot-linux-release-arm64/tests_language_string_interpolation_and_buffer_test/out.aotsnapshot

exit code:
255

stderr:
Unhandled exception:
Expect.throws<String>: Unexpected '"unreachable"'
#0      main.<anonymous closure> (file:///b/s/w/ir/cache/builder/sdk/tests/language/string/interpolation_and_buffer_test.dart:46)
#1      Expect.throws (package:expect/expect.dart:616)
#2      main (file:///b/s/w/ir/cache/builder/sdk/tests/language/string/interpolation_and_buffer_test.dart:46)
#3      _delayEntrypointInvocation.<anonymous closure> (dart:isolate-patch/isolate_patch.dart:297)
#4      _RawReceivePort._handleMessage (dart:isolate-patch/isolate_patch.dart:184)

#0      Expect._fail (package:expect/expect.dart:721)
#1      Expect.throws (package:expect/expect.dart:626)
#2      main (file:///b/s/w/ir/cache/builder/sdk/tests/language/string/interpolation_and_buffer_test.dart:46)
#3      _delayEntrypointInvocation.<anonymous closure> (dart:isolate-patch/isolate_patch.dart:297)
#4      _RawReceivePort._handleMessage (dart:isolate-patch/isolate_patch.dart:184)

--- Re-run this test:
python3 tools/test.py -n vm-aot-linux-release-arm64 language/string/interpolation_and_buffer_test
alexmarkov commented 8 months ago

Legacy variant of this test is also failing:

--- Command "vm" (took 03.000041s):
DART_CONFIGURATION=DebugX64 out/DebugX64/dart --enable_asserts --no-sound-null-safety -Dtest_runner.configuration=dartk-weak-asserts-linux-debug-x64 --ignore-unrecognized-flags --packages=/b/s/w/ir/.dart_tool/package_config.json /b/s/w/ir/tests/language/string/interpolation_and_buffer_legacy_test.dart

exit code:
255

stderr:
Unhandled exception:
Expect.throws<Error>: Unexpected '"unreachable"'
#0      main.<anonymous closure> (file:///b/s/w/ir/tests/language/string/interpolation_and_buffer_legacy_test.dart:18:35)
#1      Expect.throws (package:expect/expect.dart:616:18)
#2      main (file:///b/s/w/ir/tests/language/string/interpolation_and_buffer_legacy_test.dart:18:10)
#3      _delayEntrypointInvocation.<anonymous closure> (dart:isolate-patch/isolate_patch.dart:297:19)
#4      _RawReceivePort._handleMessage (dart:isolate-patch/isolate_patch.dart:184:12)

#0      Expect._fail (package:expect/expect.dart:721:5)
#1      Expect.throws (package:expect/expect.dart:626:7)
#2      main (file:///b/s/w/ir/tests/language/string/interpolation_and_buffer_legacy_test.dart:18:10)
#3      _delayEntrypointInvocation.<anonymous closure> (dart:isolate-patch/isolate_patch.dart:297:19)
#4      _RawReceivePort._handleMessage (dart:isolate-patch/isolate_patch.dart:184:12)

--- Re-run this test:
python3 tools/test.py -n dartk-weak-asserts-linux-debug-x64 language/string/interpolation_and_buffer_legacy_test
osa1 commented 7 months ago

I wonder if this should be fixed in CFE. The problem is the kernel for this string interpolation doesn't call toString on the parts:

print("$t ${throw "unreachable"}");

Kernel:

core::print("${t} ${throw "unreachable"}");

For the right semantics, t.toString should be called before the second part of the interpolation is evaluated.

Currently how dart2wasm handles this is, we first generate a list with all the values and pass it to a runtime function:

String._interpolate(<Object?>[t, " ", throw "unreachable"])

Since the test fails with VM as well I think it probably does the same.

For the right semantics the backends need to check each part, and if a part's static type is not String they need to call toString:

String._interpolate(<String>[t.toString(), " ", throw "unreachable"])

Instead of doing this in every backend maybe CFE could do it for the backends in one place.

(As an optimization in dart2wasm code above we could avoid allocating a Dart list, but that's a separate issue.)

osa1 commented 4 months ago

Doing this in CFE also makes optimizing the toString calls based on the static types of the concatenated parts eaiser as TFA can do it. For example if I have "$a $b where a : A and b : B, TFA can convert the toString instance calls to direct calls to A.toString and B.toString.

@chloestefantsova @johnniwinther any thoughts on doing this in CFE?

Here's a smaller repro:

class ToStringThrows {
  String toString() => throw "Throw";
}

void main() {
  var t = ToStringThrows();

  // Fails with "unreachable", but should fail with "Throw".
  print("$t ${throw "unreachable"}");
}
mraleph commented 4 months ago

See also https://github.com/dart-lang/sdk/issues/34223