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

The VM should not print CFE warnings when it is running code #34137

Open
natebosch opened this issue Aug 13, 2018 · 17 comments
Open

The VM should not print CFE warnings when it is running code #34137

natebosch opened this issue Aug 13, 2018 · 17 comments
Assignees
Labels
area-vm Use area-vm for VM related issues, including code coverage, and the AOT and JIT backends.

Comments

@natebosch
Copy link
Member

Since we are (as far as I know) considering it non-breaking to add new warnings in CFE we should not be printing warnings when running code. It's probably ok to print them when compiling to kernel only.

If I have some process which runs the Dart vm and processes the output, it would be breaking to me to have new warnings printed when I upgrade my SDK. I think the exact output of the Dart VM should be considered part of what is stable for the SDK. If we want to be able to add new warnings without breaking SDK version bumps we shouldn't print warnings during execution.

@a-siva a-siva added the area-vm Use area-vm for VM related issues, including code coverage, and the AOT and JIT backends. label Aug 14, 2018
@natebosch
Copy link
Member Author

Ping. I noticed a new warning for using ! to negate a void value which gets printed then the code runs anyway - can we suppress warnings when the VM is about to run code instead of, say, take a snapshot?

@aam
Copy link
Contributor

aam commented Sep 21, 2018

When Dart VM runs from the source(rather than from the snapshot) it does compile the source first and the warnings you see I believe are coming from the compiler.
If you create a snapshot first, then run Dart VM from that snapshot there should be no warnings.
Does the above match your experience?

@natebosch
Copy link
Member Author

Yes that matches my experience and the behavior when running from source is what I think we need to change - we should suppress the compiler output in that case. Neither running from source, nor creating an app-jit snapshot with training data should output anything on stderr from the compiler.

@aam
Copy link
Contributor

aam commented Sep 21, 2018

when running from source is what I think we need to change - we should suppress the compiler output in that case

I am not sure I understand this. Couldn't that be the only opportunity for us to give user feedback about warnings in their dart program?

@natebosch
Copy link
Member Author

Yes, if they never compile to a script snapshot or to kernel they might never see the warning from the CFE. In that case we'll rely on them running the analyzer to see it.

AFAIK we're treating new CFE warnings as non-breaking, which means we'll suddenly start changing the output of Dart programs when run from source and upgrading SDKs.

@aam
Copy link
Contributor

aam commented Sep 22, 2018

In that case we'll rely on them running the analyzer to see it.

That sounds pretty fundamental. I don't understand why we would make the case of running dart from command line ($ dart hello-world.dart) less user-friendly, why we would mandate use of analyzer to see all important output.

AFAIK we're treating new CFE warnings as non-breaking, which means we'll suddenly start changing the output of Dart programs when run from source and upgrading SDKs.

I would focus on this as there are a lot of assumptions here: 1) are we treating new CFE warnings as breaking/non-breaking? 2) Why running dart embedder from source and getting new warnings at that point is different from compiling that source explicitly and getting same new warnings there? 3) Are new warnings produced by analyzer treated differently than warnings produced by dart command line embedder? (meaning are analyzer's warnings similarly "breaking/non-breaking"?)

Answers to the questions above seem to be prerequisites and outside of scope for this blanket request for dart command line embedder to silence all CFE warnings when running from source.
If possible can you share some context for this request as I don't understand what problem you are trying to solve that existing set of tools can't solve and the only solution is for dart embedder to filter out warnings.

@kmillikin
Copy link

This is not a CFE warning, it's a Dart language error. It's mandated by https://github.com/dart-lang/sdk/blob/master/docs/language/informal/generalized-void.md, and it's definitely a breaking change.

The VM already prints warnings and errors on stderr. You should just be able to redirect that. Ultimately of course, you should try to get the error in the code you're running fixed.

Hiding errors when running code seems like a usability problem. Some errors are (will be) fatal and there's no way to run the code at all. In those cases at least, we need to give the user some indication.

@natebosch
Copy link
Member Author

are we treating new CFE warnings as breaking/non-breaking?

I've seen them show up without any note at all the changelog - let alone a breaking change notification. In some conversations with @leafpetersen it was my understanding that we'd treat new errors as breaking (if they don't already exist in the analyzer as errors), and new warnings as non-breaking.

Why running dart embedder from source and getting new warnings at that point is different from compiling that source explicitly and getting same new warnings there?

When executing code stderr may have meaning that is determined by the author, not by the Dart team. When compiling explicitly stderr has meaning determined only by the Dart team.

Are new warnings produced by analyzer treated differently than warnings produced by dart command line embedder? (meaning are analyzer's warnings similarly "breaking/non-breaking"?)

AFAIK new warnings in either are considered non breaking externally - the code still runs and maintains the same semantics. They are breaking internally but we have tolerance for adding new static checking without a breaking version bump in the SDK because we control the code and can migrate it.

If possible can you share some context for this request as I don't understand what problem you are trying to solve that existing set of tools can't solve and the only solution is for dart embedder to filter out warnings.

I have tools written in Dart where stderr has specific meaning. In some cases any output means "this tool is broken and you can't trust it". I sometimes need to hop between versions of the SDK and running from source is smoother than constantly recompiling for each version and running from kernel. When I upgrade my Dart SDK and these tools suddenly appear broken it means I must edit my code, or worse potentially code in dependencies I don't own, in order to continue to use the tool. I consider forcing a change in the code in order to get the same behavior as a prior version to be a breaking change.

This is not a CFE warning, it's a Dart language error.

It wasn't my intention to focus on any particular warning or error. Another case where this came up had to do with mandating new return statements in some methods. If I recall correctly that was defined as a warning by the language team.

The VM already prints warnings and errors on stderr. You should just be able to redirect that.

I can't redirect it if stderr already has a meaning in my program.

Hiding errors when running code seems like a usability problem. Some errors are (will be) fatal and there's no way to run the code at all. In those cases at least, we need to give the user some indication.

Yes, that's why I've specifically focused this issue on warnings. If we can't run the code - and by definition we can't if there are errors - then printing to stderr is the most sensible thing to do. We're already aware as a team of the impact of adding new errors and making code not run at all - the issue is that we are changing program behavior on SDK upgrades by adding warnings which have not been treated with the same caution as errors.

@aam
Copy link
Contributor

aam commented Sep 24, 2018

Thanks for the details.

When executing code stderr may have meaning that is determined by the author, not by the Dart team. > When compiling explicitly stderr has meaning determined only by the Dart team.

I understand what you mean, but I don't think that this is accurate. "When executing code" includes compilation and that might the only channel for the compiler to provide diagnostic to the user( see my $ dart hello_world.dart example.

I have tools written in Dart where stderr has specific meaning.

Can you make your tool more robust in a sense of ignoring stderr(if that is what you desire) until your code actually starts executing?

@natebosch
Copy link
Member Author

I understand what you mean, but I don't think that this is accurate. "When executing code" includes compilation and that might the only channel for the compiler to provide diagnostic to the user

I think my point is that these particular diagnostics (warnings that don't impact runtime behavior, or even those that would impact runtime behavior but only if a particular branch is hit) are not useful enough to the user to warrant changing the semantics of the application being run by changing what is output on stderr. I tried to find some prior art here and mostly came up short.

  • Languages like python and ruby follow the Dart 1 model. They don't print to stderr until there is some bug that causes a runtime issue - they don't concern themselves with static warnings.
  • Swift has a swift run but it always prints that it is compiling, it doesn't stay silent until there are warnings. I don't think that's what we want dart to do.
  • Go has go run but the language does not have any warnings, it only has errors that prevent execution entirely.
  • Most other compile-and-run languages don't seem to support a single command model.

Can you make your tool more robust in a sense of ignoring stderr(if that is what you desire) until your code actually starts executing?

It depends on the tool, I might not own all the tools that are executing programs and reading from stderr.

@aam
Copy link
Contributor

aam commented Sep 24, 2018

It depends on the tool, I might not own all the tools that are executing programs and reading from stderr.

I think your best bet then would be to do the compilation separately and feed resultant kernel to the vm.

@aam
Copy link
Contributor

aam commented Oct 9, 2018

I'm sorry, I just discovered that standalone dart embedder has --suppress-fe-warnings option that does what you want already:

╰─➤  xcodebuild/DebugX64/dart-sdk/bin/dart --suppress_fe_warnings hello.dart
Hello, world!
╰─➤  xcodebuild/DebugX64/dart-sdk/bin/dart  hello.dart                      
hello.dart:4:3: Warning: Must explicitly return a value from a non-void function.
  return;
  ^
Hello, world!
╰─➤  cat hello.dart
int main() {
  print('Hello, world!');
  return 0;
  return;
}

So that makes discussion above slightly irrelevant. Again, sorry about that.

@lrhn
Copy link
Member

lrhn commented Oct 9, 2018

To me it seems like the issue here is that we have two different modes of working with the same dart executable.
One is running a production program, the other is running code while developing.
In the latter case you do want to be told about warnings. In the former case, we should assume you are aware of the warning and just want to run the program (which may emit meaningful messages on stderr).

No single behavior of the dart executable can satisfy both behaviors.
The simplest solution, as already noticed above, is to have a flag. The next question is then which behavior is the default, and which behavior requires the flag.

The discoverability of the flag is low. The usability impact of not displaying warnings is high (we assume most users will want to fix the warnings). That suggests that the default should be to show warnings.

There is a cost to this because people running in production might not be aware of the flag until they get hit by a new warning. Adding warnings is non-breaking for the language (they do not change the behavior of any running program), but not necessarily for the dart executable because it mixes compile-time and run-time behavior and the run-time behavior of the program might include the output on stderr.

A different, possibly more practical, solution could be to have two different dart executables, dart-dev and dart, where dart-dev prints warnings and enables assertions by default, and dart does not.
(Or, in the traditional Unix way, it's just one executable that dispatches on the name it's invoked as).
Or it could have a --dev flag that does the same things.

@natebosch
Copy link
Member Author

I just discovered that standalone dart embedder has --suppress-fe-warnings option that does what you want already

Awesome! That definitely helps. I'm happy to see further consideration of changing the default. My preference skews towards suppressing by default, but given that the flag exists I'm not prepared to argue quite as strongly. We could also consider defaulting this for pub run in the (somewhat rare) cases of running from source. #51033

Or it could have a --dev flag that does the same things.

I think this has come up before - the idea of having a single flag to wrap up the things a developer would generally use - like --enable-asserts, --enable-fe-warnings (assuming we change the default), as well as skipping some optimizations that we might want in "production". I can't find the issue but I think there was one around using more memory than necessary in case observatory was turned on, we could consider only allowing observatory in --dev mode and saving the memory otherwise.

@leafpetersen
Copy link
Member

I think having a flag to suppress warnings is the right thing here. For what it's worth, that seems to be what java does, we could look at other languages. For programs that parse output, the best practice should be to run with that flag.

@lrhn
Copy link
Member

lrhn commented Oct 12, 2018

The --disable-fe-warnings flag is a valid solution. I'd argue that the name is overly specific, and it should probably only be called --disable-warnings or something similar, if we intend to make it generally available and recommend its use. Users should not need to know that there is a "front end".

@munificent
Copy link
Member

We have changed the language specification so that tools are no longer required to report all specified warnings. More context is here. If CFE or the VM no longer want to report warnings, they are now free to do so while still respecting the language specification.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-vm Use area-vm for VM related issues, including code coverage, and the AOT and JIT backends.
Projects
None yet
Development

No branches or pull requests

7 participants