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

Add runtime checks for the statements which can be optimized to true or false #1243

Closed
sgrekhov opened this issue Dec 3, 2021 · 5 comments
Closed
Assignees
Labels
type-enhancement A request for a change that isn't a bug

Comments

@sgrekhov
Copy link
Contributor

sgrekhov commented Dec 3, 2021

We have some checks like

  Expect.isTrue(c is Object);

These statements are optimized by dart2js to Expect_isTrue(true);. Most probably, these statements are optimized in the similar way by AOT. We need to add some runtime checkings here to test not only compiler optimizations but the runtime as well.

To do that find all of the tests that have analyzer hints like hint - Unnecessary type check; the result is always 'true'. and add some runtime checks

@sgrekhov sgrekhov added the type-enhancement A request for a change that isn't a bug label Dec 3, 2021
@eernstg
Copy link
Member

eernstg commented Dec 3, 2021

Probably (c is dynamic) is Object will do.

@sgrekhov
Copy link
Contributor Author

sgrekhov commented Dec 3, 2021

@eernstg do you mean (c as dynamic) is Object?

@eernstg
Copy link
Member

eernstg commented Dec 3, 2021

Yes, thanks!

@sgrekhov
Copy link
Contributor Author

sgrekhov commented Dec 3, 2021

Thank you. But dart2js is too smart, it optimizes it to Expect_isTrue(true); as well :). I'd suggest something like

class C {}
class D extends C {}

main() {
  Expect.isTrue(D() is C);  // Optimized to Expect_isTrue(true);
  Expect.isTrue(((Object o) {return o;})(D()) is C); // Does runtime check in JS
}

@rakudrama
Copy link
Member

The way to outwit dart2js is to make things sufficiently indirect, and have possible true and false results.

class C {}
class D extends C {}

void checkIsD(bool expected, Object? o) {
  Expect.equals(expected, o is D);
}

void checkIsC(bool expected, Object? o) {
  Expect.equals(expected, o is C);
}

void checkIs<T>(bool expected, Object? o) {
  Expect.equals(expected, o is T);
}

void directChecks() {  // Most of these tests are inlined and simplified
  checkIsD(true, D());
  checkIsD(false, C());
  checkIsD(false, Object());
  checkIsC(true, D());
  checkIsC(true, C());
  checkIsC(false, Object());
  checkIs<D>(true, D());
  checkIs<D>(false, C());
  checkIs<D>(false, Object());
  checkIs<C>(true, D());
  checkIs<C>(true, C());
  checkIs<C>(false, Object());
}

@pragma('dart2js:noInline')
void check(void Function(bool, Object?) checker, bool expected, Object? o) {
  checker(expected, o);
}

void indirectChecks() { // Indirect calls via non-inlined function prevent the compiler reducing the code to the answer
  check(checkIsD, true, D());
  check(checkIsD, false, C());
  check(checkIsD, false, Object());
  check(checkIsC, true, D());
  check(checkIsC, true, C());
  check(checkIsC, false, Object());
  check(checkIs<D>, true, D());
  check(checkIs<D>, false, C());
  check(checkIs<D>, false, Object());
  check(checkIs<C>, true, D());
  check(checkIs<C>, true, C());
  check(checkIs<C>, false, Object());
}

@sgrekhov sgrekhov self-assigned this Dec 13, 2021
sgrekhov pushed a commit that referenced this issue Dec 13, 2021
sgrekhov pushed a commit that referenced this issue Dec 14, 2021
sgrekhov pushed a commit that referenced this issue Dec 15, 2021
sgrekhov pushed a commit that referenced this issue Dec 16, 2021
sgrekhov pushed a commit that referenced this issue Dec 17, 2021
sgrekhov pushed a commit that referenced this issue Dec 20, 2021
…ctor tear-offs and Generic functions as type arguments
sgrekhov pushed a commit that referenced this issue Dec 24, 2021
copybara-service bot pushed a commit to dart-lang/sdk that referenced this issue Jan 13, 2022
2022-01-12 [email protected] dart-lang/co19#1259. Roll failures fixed
2022-01-12 [email protected] dart-lang/co19#1258. Enhanced Enums semantics tests rewritten according to changed spec
2022-01-12 [email protected] Fixes dart-lang/co19#1261. Expected result value fixed
2022-01-11 [email protected] dart-lang/co19#1259. Different roll failures fixed (mainly typos)
2022-01-11 [email protected] Fixes dart-lang/co19#1247. Allow positional parameters with the different name than in superclass
2022-01-10 [email protected] Fixes dart-lang/co19#1248. Add additional error expectation
2022-01-10 [email protected] Fixes dart-lang/co19#1256. Use correct type of super parameters. Expect an error if the type is wrong
2022-01-10 [email protected] Fixes dart-lang/co19#1255. Use nullable type parameter for type check
2022-01-10 [email protected] Fixes dart-lang/co19#1254. Typo fixed
2022-01-10 [email protected] Fixes dart-lang/co19#1253. Typo fixed
2022-01-10 [email protected] Fixes dart-lang/co19#1252. Typo fixed
2022-01-10 [email protected] Fixes dart-lang/co19#1251. Default value is added to non-nullable optional argument
2022-01-10 [email protected] Fixes dart-lang/co19#1250. Named parameter made named in a constructor call
2022-01-10 [email protected] Fixes dart-lang/co19#1249. Missed superconstructor added
2022-01-10 [email protected] Fixes dart-lang/co19#1246. Use correct type for type inference
2021-12-28 [email protected] Avoid sender making more progress than the receiver in Isolate/pause_A01_t01/2. (dart-lang/co19#1242)
2021-12-28 [email protected] Fixes dart-lang/co19#1243. Runtime type checks added to typed_data tests
2021-12-27 [email protected] dart-lang/co19#1243 Runtime type checks added to io and match tests
2021-12-24 [email protected] dart-lang/co19#1243 Runtime type checks added
2021-12-23 [email protected] dart-lang/co19#1243 Runtime type checks added to Spread collections tests
2021-12-23 [email protected] dart-lang/co19#1243 Runtime type checks moved to Expect class
2021-12-23 [email protected] dart-lang/co19#1243 Runtime type checks added to Set literals tests
2021-12-23 [email protected] dart-lang/co19#1243 Runtime type checks added to Expect class
2021-12-22 [email protected] dart-lang/co19#1243 Runtime type checks added for nnbd tests
2021-12-22 [email protected] dart-lang/co19#1244 CHECK_TOP_MERGE replaced by CheckTopMerge
2021-12-22 [email protected] dart-lang/co19#1243 Runtime type checks added for nnbd tests
2021-12-21 [email protected] Fixes dart-lang/co19#1245. Fix checking type of Enum members
2021-12-20 [email protected] dart-lang/co19#1243 Runtime type checks added for Control flow collections, Constructor tear-offs and Generic functions as type arguments
2021-12-17 [email protected] dart-lang/co19#1243 Runtime checks added for statements which can be optimized to 'true' or 'false'
2021-12-16 [email protected] Issue dart-lang/co19#1244: Utils functions updated according to the Effective Dart Guide, relative tests updated accordingly.
2021-12-16 [email protected] dart-lang/co19#1243 Runtime checks added for statements which can be optimized to 'true' or 'false'
2021-12-15 [email protected] dart-lang/co19#1243 Runtime checks added for statements which can be optimized to 'true' or 'false'
2021-12-15 [email protected] dart-lang/co19#1243 Expect.isTrue() is returned back for type checking
2021-12-14 [email protected] dart-lang/co19#1243 Runtime checks added for statements which can be optimized to 'true' or 'false'
2021-12-13 [email protected] Issue dart-lang/co19#1094: minor improvement for Utils/test_mode_check
2021-12-13 [email protected] dart-lang/co19#1243 Runtime checks added for statements which can be optimized to 'true' or 'false'

Cq-Include-Trybots: dart/try:analyzer-nnbd-linux-release-try,dart2js-nnbd-linux-x64-chrome-try,ddc-nnbd-linux-release-chrome-try,front-end-nnbd-linux-release-x64-try,vm-kernel-nnbd-linux-release-x64-try,vm-kernel-nnbd-win-release-x64-try,vm-kernel-precomp-nnbd-linux-release-x64-try
Change-Id: I8b2b346600c03061b8b735be874c3f377d70b354
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/227800
Reviewed-by: William Hesse <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type-enhancement A request for a change that isn't a bug
Projects
None yet
Development

No branches or pull requests

3 participants