#3180. Add JSExport tests. Part 4. #3253
Conversation
|
I guess that |
| } | ||
| } | ||
|
|
||
| @JSExport() |
There was a problem hiding this comment.
This could be a warning so users don't think it makes a difference, but I don't even know if this declaration appears in the CFE, so we might not be able to bubble up a warning here.
IIRC, typedefs are resolved early, so we would never even see CAlias as a type in the kernel.
There was a problem hiding this comment.
@johnniwinther, perhaps you could help us a bit here—does the generated Kernel code allow a backend (in particular, a backend compiling to JS) to detect that there is a type alias whose metadata includes a JSExport? Otherwise, we probably can't emit a warning that this metadata has no effect.
There was a problem hiding this comment.
Hi, @johnniwinther, I can see that you're on vacation right now. Hope you're having a great time! ;-)
We're planning to land this PR now, but it would still be helpful to get your comment on the above question when you're back.
In particular, it looks like backends can't emit diagnostic messages about metadata on type aliases, because the type aliases are gone when the program has been translated to Kernel code. There may also be other situations where backends just can't emit diagnostic messages because the information is gone.
Do you see a way to allow backends to improve on this situation? Perhaps the declaration of JSExport could have a pragma specifying the kinds of declarations that are appropriate for this type to be used as metadata?
eernstg
left a comment
eernstg
left a comment
There was a problem hiding this comment.
Looks good! I think it would be helpful to add a comment about the tests that seem to assume that we can do super invocations of members of another object (like otherObject.super.m(), or even otherObject.super<SomeType>.m() because every superinvocation must specify a class from which the search should start, except that this is implicit when the superinvocation occurs inside that class).
One more thing came up, and I asked for input from @johnniwinther in order to clarify the situation around type aliases and Kernel code.
| } | ||
| } | ||
|
|
||
| @JSExport() |
There was a problem hiding this comment.
@johnniwinther, perhaps you could help us a bit here—does the generated Kernel code allow a backend (in particular, a backend compiling to JS) to detect that there is a type alias whose metadata includes a JSExport? Otherwise, we probably can't emit a warning that this metadata has no effect.
sgrekhov
left a comment
sgrekhov
left a comment
There was a problem hiding this comment.
Typos fixed and the description updated. PTAL.
|
We could proceed as follows: All the errors/warnings that we'd ideally want to have are expected in these tests. Some of those expectations may fail because the implementation of the diagnostics hasn't been completed at this time. Others may fail because it's difficult to do at all (e.g., type aliases may just be gone completely in Kernel code, and it might be a major refactoring effort to change that). So we'd just approve all the failures with references to suitable issues. Some tests would then start working as soon as the diagnostics have been implemented. For other tests, we'd clarify the situation; if the desired diagnostic turns out to be impractical to emit (e.g., errors/warnings about type aliases) then the test expectations would be adjusted accordingly. With a plan along those lines, we could land this PR now. @sgrekhov, is it true that every desired diagnostic message is expected at this time, or do we have some locations where no message is expected even though we'd like to have it? |
|
In this PR |
eernstg
left a comment
eernstg
left a comment
There was a problem hiding this comment.
Added a couple of comments. I found one location where I believe we should expect a diagnostic message (following the plan I suggested).
| } | ||
| } | ||
|
|
||
| @JSExport() |
There was a problem hiding this comment.
Hi, @johnniwinther, I can see that you're on vacation right now. Hope you're having a great time! ;-)
We're planning to land this PR now, but it would still be helpful to get your comment on the above question when you're back.
In particular, it looks like backends can't emit diagnostic messages about metadata on type aliases, because the type aliases are gone when the program has been translated to Kernel code. There may also be other situations where backends just can't emit diagnostic messages because the information is gone.
Do you see a way to allow backends to improve on this situation? Perhaps the declaration of JSExport could have a pragma specifying the kinds of declarations that are appropriate for this type to be used as metadata?
| typedef void Foo(); | ||
| // ^^^ | ||
| // [analyzer] unspecified | ||
| // [web] unspecified |
There was a problem hiding this comment.
This expectation is good: It fails for now, and we may not be able to fix it, but right now we just want to expect all the diagnostic messages that we want to have.
There was a problem hiding this comment.
It should be possible for backends to report errors on typedef declarations but not uses through a typedef. So it should be possible to report an error here.
OK, so they would be in the same category: We want those diagnostic messages because |
|
Tests updated to expect a warning in case of typedef annotated with |
Fix typo in comment
Fix typo in comment
2025-07-17 sgrekhov22@gmail.com dart-lang/co19#3180. Add `@staticInterop` tests. Part 1. (dart-lang/co19#3256) 2025-07-15 sgrekhov22@gmail.com dart-lang/co19#3180. Add tests checking `name` value (dart-lang/co19#3255) 2025-07-15 sgrekhov22@gmail.com dart-lang/co19#3180. Add `anonymous` annotation tests (dart-lang/co19#3254) 2025-07-15 sgrekhov22@gmail.com dart-lang/co19#3180. Add JSExport tests. Part 4. (dart-lang/co19#3253) Cq-Include-Trybots: luci.dart.try:analyzer-linux-release-try,dart2js-minified-linux-d8-try Change-Id: Ice01c51e219f0ca68f92390db029ca3f8a65b7a8 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/441140 Reviewed-by: Chloe Stefantsova <cstefantsova@google.com> Commit-Queue: Erik Ernst <eernst@google.com> Reviewed-by: Erik Ernst <eernst@google.com>
4 participants
No description provided.