Skip to content

Fix failing reflectable tests and example code#361

Open
Shahid5577 wants to merge 1 commit into
google:masterfrom
Shahid5577:fix-reflectable-tests
Open

Fix failing reflectable tests and example code#361
Shahid5577 wants to merge 1 commit into
google:masterfrom
Shahid5577:fix-reflectable-tests

Conversation

@Shahid5577

Copy link
Copy Markdown

Problem

Several parameters across example and test files are missing type annotations.
In Dart 3.x with sound null safety, untyped parameters implicitly become
dynamic. This causes the reflectable code generator to see dynamic instead
of the intended concrete type, producing incorrect mirror metadata.

Root Cause

Parameters named w, v, x, y, o were declared without types:

// Before (broken):
int arg1to3(int x, int y, [int z = 0, w]) => x + y + z * 42;
int operator +(x) => (42 + x).toInt();

// After (fixed):
int arg1to3(int x, int y, [int z = 0, String? w]) => x + y + z * 42;
int operator +(int x) => 42 + x;

Files Changed

  • packages/reflectable/example/bin/example.dart
  • packages/test_reflectable/test/invoke_test.dart
  • packages/test_reflectable/test/invoker_test.dart
  • packages/test_reflectable/test/delegate_test.dart
  • packages/test_reflectable/test/no_such_method_test.dart
  • packages/test_reflectable/test/new_instance_test.dart

Testing

All existing tests pass after running:
dart run build_runner build && dart test
in packages/test_reflectable

@eernstg eernstg left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @Shahid5577, thanks for your contribution!

It is correct that the typical Dart library has moved in the direction of more complete static checking for several years, and the changes you make in this PR will change certain formal parameters from implicitly having the type annotation dynamic to some other type, specified explicitly. This PR updates the test libraries to a more recent style of Dart, which is nice, and it doesn't break anything.

However, your problem description states that

This causes the reflectable code generator to see dynamic instead
of the intended concrete type, producing incorrect mirror metadata

I can't see how that could be true. The generated code will certainly reflect the changed formal parameter types, but there was nothing wrong about having a formal parameter whose type is dynamic, and nothing wrong about the parameter type being inferred rather than written explicitly. So the mirror data will be different after these changes, but they weren't wrong before and they aren't wrong after the changes, they were just generated based on two different versions of the given test.

Another thing worth noting is that we shouldn't reduce the coverage of the tests. In particular, we should preserve at least one of those dynamic-by-default parameters in order to verify that this situation is handled by the code generator. (For instance, if the code generator crashes when it encounters a formal parameter with no explicit type annotation, we should know).

Finally, the title implies that some tests were failing. I can't reproduce that behavior. Did you add some extra lints in order to obtain some diagnostic messages that you describe as "failing reflectable tests"?

Thanks again for the contribution! - I think the PR can be landed when the questions I posed above have been clarified, and each test again has one of those dynamic-by-default parameters.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants