Skip to content

Is DDC expected to emit static warnings? #36123

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

Closed
sortie opened this issue Mar 6, 2019 · 6 comments
Closed

Is DDC expected to emit static warnings? #36123

sortie opened this issue Mar 6, 2019 · 6 comments
Assignees
Labels
area-infrastructure Use area-infrastructure for SDK infrastructure issues, like continuous integration bot changes. area-web-js Issues related to JavaScript support for Dart Web, including DDC, dart2js, and JS interop. closed-obsolete Closed as the reported issue is no longer relevant P2 A bug or feature request we're likely to work on type-bug Incorrect behavior (everything from a crash to more subtle misbehavior) web-dev-compiler

Comments

@sortie
Copy link
Contributor

sortie commented Mar 6, 2019

@eernstg noticed that the expectation for tests annotated static warning are only StaticWarning on the analyzer, however the expectation is pass on anything else. He wonders if DDC is supposed to emit static warnings, and thus should have a different expectation. The current logic is in tools/testing/dart/test_runner.dart is:

  Expectation get realExpected {
  ...
    if (configuration.compiler == Compiler.dart2analyzer && hasStaticWarning) {
      return Expectation.staticWarning;
    }
    return Expectation.pass;
  }

Once @eernstg finds out whether DDC is supposed to emit static warnings, then we can change this piece of code to set the right expectation for the tests.

@sortie sortie added area-infrastructure Use area-infrastructure for SDK infrastructure issues, like continuous integration bot changes. P2 A bug or feature request we're likely to work on type-bug Incorrect behavior (everything from a crash to more subtle misbehavior) labels Mar 6, 2019
@eernstg
Copy link
Member

eernstg commented Mar 6, 2019

Awaiting input from the DDC team (asked @jmesserly and others).

@eernstg
Copy link
Member

eernstg commented Mar 6, 2019

Cf. #36126 and #36127, whose resolution may depend on actions taken here.

@jmesserly
Copy link

jmesserly commented Mar 13, 2019

Hi I'm back from vacation.

Currently DDC emits the same messages as its front end (Analyzer or CFE). I don't know to what extent CFE is expected to emit the same warnings as Analyzer.

Personally, I don't think we need to test static warnings on DDC, because they're already covered by the other bots, and DDC isn't the one producing those messages.

@eernstg
Copy link
Member

eernstg commented Mar 14, 2019

Welcome back! ;-)

It doesn't actually resolve the issue to determine that DDC/DDK uses a specific front end, or to know whether those warning messages are printed.

The issue is whether test.py will see the DDC/DDK subprocess as reporting pass or something else (by terminating with a different exit code or whatever it is that test.py and its ilk use to determine the actual outcome) when the expectation in the test is static type warning (static warning isn't supported, so they are all ..type.. warnings).

Currently, where the analyzer does not (yet) use the common front end, and every other tool is expected to report pass in the situation where the test expects static type warning, there is no test whatsoever on the correctness of the warnings emitted by the CFE.

Maybe it's OK to leave this behavior completely untested in the CFE until the analyzer switches over, but it seems reasonable to make that decision, rather than letting it happen by accident.

We would then consider the DDC/DDK as "backend-ish" tools, in the sense that test.py should continue to expect the outcome pass when the test specifies the expectation static type warning. Basically, they are expected to ignore all warnings, as seen from test.py. In that case we'd just conclude that everything is fine as it is today, and close this issue.

@srawlins srawlins added area-web-js Issues related to JavaScript support for Dart Web, including DDC, dart2js, and JS interop. web-dev-compiler labels Oct 21, 2019
@nshahan
Copy link
Contributor

nshahan commented Dec 28, 2023

I just came across this old issue and I'm not sure if it is still an open question or not. I'm not familiar with the term "static warnings" are there any examples of tests that produce some?

I can say that there are some static errors that are only generated when compiling for the JavaScript backends and tests for these are run on the dart2js and ddc test configurations. They appear in static error test files marked with the // [web] ... comments.

Example: https://github.com/dart-lang/sdk/blob/main/tests/lib/js/static_interop_test/js_types_static_errors_test.dart

@sigmundch
Copy link
Member

I believe we can close this issue as obsolete. I'll mark it as such, but anyone please feel free to reopen if I'm mistaken.

Long ago, we used to provide errors and warnings. The language has evolved to mostly provide errors, and our tests mostly provide coverage for errors. Back then, we also only tested for static errors and warnings with tests and multi-tests containing the "compile-time error" and "static type warning" annotations in the outcome comments. We evolved from that, and moved as much of the test coverage for static diagnostics to the static_test infrastructure. The test you liked, is one example of that. The reason for such shift is that static_test added context and precise information about what and where an error was reported. Prior to it, we couldn't tell whether a test failed for the right reasons, and we found ourselves with regressions because of that improper coverage.

Static tests can be run by any configuration, but initially it was only valuable to do so for analyzer and CFE (as the original doc suggests), since they provided all of the diagnostics. Support for web-specific errors was added when we introduced the js-interop transformer. This kernel transformer provides a rich set of errors today and required static coverage. Because of that, now these tests are run by the web backends too. We need to update the docs above to highlight this, though.

@sigmundch sigmundch added the closed-obsolete Closed as the reported issue is no longer relevant label Jan 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-infrastructure Use area-infrastructure for SDK infrastructure issues, like continuous integration bot changes. area-web-js Issues related to JavaScript support for Dart Web, including DDC, dart2js, and JS interop. closed-obsolete Closed as the reported issue is no longer relevant P2 A bug or feature request we're likely to work on type-bug Incorrect behavior (everything from a crash to more subtle misbehavior) web-dev-compiler
Projects
None yet
Development

No branches or pull requests

6 participants