Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2852533003:
Change specification of main. (Closed)
|
Created:
3 years, 7 months ago by Lasse Reichstein Nielsen Modified:
3 years, 6 months ago CC:
reviews_dartlang.org, Bob Nystrom, Leaf Target Ref:
refs/heads/master Visibility:
Public. |
DescriptionChange specification of main.
R=eernst@google.com, rnystrom@google.com
Committed: https://github.com/dart-lang/sdk/commit/13f3df7e0cd4de9c99d344f242afb8514ada06f7
Patch Set 1 #Patch Set 2 : Add ref. #Patch Set 3 : Add clause saying that a non-script causes a compile-time error if a script is expected. #Patch Set 4 : Fix typo and phrasing. #
Total comments: 19
Patch Set 5 : Address comments. #Messages
Total messages: 26 (4 generated)
lrn@google.com changed reviewers: + eernst@google.com, floitsch@google.com
I think this matches what we agreed on on Tuesday. There is no specification of what happens if you try to execute a non-script, it's just unspecified behavior. That means that dart2js can just fail to compile, saying that its entry-point is not a script. A Isolate.spawnUri call should report back if its argument wasn't a script. In checked mode, the call can still fail due to type parameters on the main function, but that should be considered a runtime error. In Strong-mode, we could choose to reject a main function where the first argument isn't a supertype of List<String>.
ahe@google.com changed reviewers: + ahe@google.com
I don't find the change clear on what happens if a library isn't a script.
That's deliberate. Nothing happens if a library isn't a script, most libraries aren't. There is no runtime-semantics associated with using a non-script where a script is expected, it's just wrong. It's no different from having a top-level syntax error in your library - it never reaches the point where it can start executing, and it fails before any user code starts to execute. It doesn't fail at *runtime*. Whoever expected the script is free to tell the user that they are doing something wrong. This is different from what was previously there, where you had to (potentially) execute a user-written `main` getter before even knowing if you could call the function (or if it even was a function). Which was generality for no apparent reason. So, if you do try to use a non-script as a script, then it's a bad input. There are two cases for that: - It's a tool that expects a script. It should tell the user that the input isn't correct. That's what dart2js would do. So should the VM if you try to start it with a non-script. It would be silly for dart2js to successfully compile the program into something that immediately fails when run (which is what should happen if we specified it as a runtime error). - It's Isolate.spawnUri (the only runtime-thing that cares about a script). It should report an error, most likely in the returned future.
If it isn't a compile-time error, dart2js will have to generate a program that throws a runtime error.
Fasta also needs to know what to generate: a program with that reports a compile-time error or a runtime error.
If we have to be that specific, it'll have to be a compile-time error. However, I can't find anything in the specification that *explicitly* says that a syntax error causes a compile-time error. It's just a problem that the input to the compiler is not a valid Dart program, a problem that precludes execution. Trying to compile something that is not a valid Dart program isn't covered by the Dart specification, but it is implicitly a compile-time error - because the specification is considered as specifying every valid program and every valid execution, so something that isn't specified isn't a valid program.. In the same way, executing a library that is not a script is not covered, it is implicitly a compile-time error, because it's not specified.
And obviously that only applies to tools that *require* a script. A compiler like DDC that compiles libraries individually can compile any library. Trying to execute one of those as a script will then need to check the compiled library for whether it contains a `main` method, and refuse to execute it otherwise. That's not a *compile-time* error, compilation ran without a hitch. It's an "attempt to run script" error, independent of compile-time and runtime, it's just an invalid argument to the "run compiled script" tool.
On 2017/04/28 12:57:02, Lasse Reichstein Nielsen wrote: > And obviously that only applies to tools that *require* a script. > A compiler like DDC that compiles libraries individually can compile any > library. It also depends on your definition of a compiler, and you have to be careful what you mean when you say compiler. An AOT compiler like dart2js will behave differently than DDC that's more of a hybrid. For example, AFAIK, DDC generates code that evaluates compile-time constants at runtime after DDC has finished running. So a DDC compilation isn't technically done when the program is ready to run, and you may get a compile-time error at runtime.
On 2017/04/28 14:55:58, ahe wrote: ... > It also depends on your definition of a compiler, and you have to be careful > what you mean when you say compiler. Acknowledged. Which is yet another reason to not over-specify, it often comes with unwarranted assumptions. So, I still think the best way to handle this is to say that trying to run a non-script is neither a runtime error nor compile-time error, it's a malformed input to whatever is expecting a script. The library is not a valid Dart script, and that is the error, not that it doesn't compile (it might, as a library), or that it fails at runtime (it never gets to that). It's not a script, and you do need to have some amount of compilation before being able to recognize that - at least enough to parse the library and its exports and recognize that there is exactly one exported declaration named "main" and it's a function declaration with zero, one or two required positional parameters. It's somewhat similar to `int.parse`. If you give it a malformed string, it throws a "FormatException", corresponding to a compilation error. If you pass it a wellformed string, it evaluates to a valid number. If you pass it `null`, it doesn't even try, it's just not a valid argument. That's what giving a non-script to dart2js or the VM is, just not a valid argument.
On 2017/05/01 05:47:03, Lasse Reichstein Nielsen wrote: > On 2017/04/28 14:55:58, ahe wrote: > ... > > It also depends on your definition of a compiler, and you have to be careful > > what you mean when you say compiler. > > Acknowledged. > Which is yet another reason to not over-specify, it often comes with unwarranted > assumptions. "First, $S$ is compiled as a library as specified above." > So, I still think the best way to handle this is to say that trying to run a > non-script is neither a runtime error nor compile-time error, it's a malformed > input to whatever is expecting a script. I don't think it's helpful or valuable to add another kind of error we must test for. If this can't be categorized as a compile-time error, we need to extend our test framework to recognize this situation so we can test that our tools handle this particular situation correctly.
PTAL
rnystrom@google.com changed reviewers: + rnystrom@google.com
LGTM.
LGTM, with some recommendations for changes. https://codereview.chromium.org/2852533003/diff/60001/docs/language/dartLangS... File docs/language/dartLangSpec.tex (right): https://codereview.chromium.org/2852533003/diff/60001/docs/language/dartLangS... docs/language/dartLangSpec.tex:7164: that can be called with zero, one or two positional arguments. So the situation before: A library containing `main(a,b,c){}` is a script and there is a static warning (because it takes >2 required arguments). The situation with this change: A library containing `main(a,b,c){}` is not a script (and there are no diagnostic messages of any kind). This means that separate compilation cannot detect any problems with a declaration like `main(a,b,c){}`. To me that looks like a step backwards. Given that we have had a warning in this case until now (except that the analyzer doesn't detect it ;-), couldn't we keep it flagged (and maybe make it an error)? How is it going to help people that they can have these "bad main functions"? To keep it, we could have `A {\em script} is a library whose exported namespace (\ref{exports}) includes a top-level function declaration named \code{main}. It is a static warning if it cannot be called with zero, one, or two positional arguments.` There's a small formatting thing, too: I'm used to grepping the LaTeX source for relevant parts of the spec on various topics. That will slowly get more and more error-prone if we insert newlines in phrases (e.g., if we start formatting the spec in 80 columns, maybe even using auto-fill-mode in Emacs). I can see that the lines above are somewhat phrase-oriented (one line contains "one meaning"), but I might well be searching for 'includes a top-level', which won't work any more. Because of this, I've avoided reformatting to 80 columns in a lot of cases where I touched the spec source, in order to keep obvious "units of meaning" on a single line, e.g., by having one whole sentence per line. In this particular case I'd keep the whole sentence on one line, and in general I would prefer a somewhat more conservative approach to the insertion of newlines. https://codereview.chromium.org/2852533003/diff/60001/docs/language/dartLangS... docs/language/dartLangSpec.tex:7170: Then, the top-level function defined by \code{main} An exported namespace is a mapping that binds names to declarations. We can't really invoke a declaration, but I think we do have precedence for talking about a declaration and the semantic entity that its name denotes at run time as the same thing. So let's say that there is a top-level function which is bound to \code{main} in that namespace. I think 'the top-level function \code{main}` would then be OK; also `the top-level function bound to \code{main} in the exported namespace..` and (of course) `the top-level function defined by the declaration bound to \code{main} in the exported namespace..`. But `the top-level function defined by \code{main}..` seems to use `defined by` to refer to the declaration, and then instead proceeds with `\code{main}` which is clearly the name. I think that's more twisted than it needs to be. https://codereview.chromium.org/2852533003/diff/60001/docs/language/dartLangS... docs/language/dartLangSpec.tex:7172: with up to two arguments, depending on how many it will accept. The 'depending ..' is too imprecise. Just `up to two arguments: If \code{main}..` is better. https://codereview.chromium.org/2852533003/diff/60001/docs/language/dartLangS... docs/language/dartLangSpec.tex:7183: it is invoked an object whose runtime type implements \code{List<String>} `invoked [with] an object` https://codereview.chromium.org/2852533003/diff/60001/docs/language/dartLangS... docs/language/dartLangSpec.tex:7190: the library is not considered a script. If we keep the approach where `main(a,b,c){}` is detected and flagged at compile-time then this is no longer true. I believe it can be omitted. https://codereview.chromium.org/2852533003/diff/60001/docs/language/dartLangS... docs/language/dartLangSpec.tex:7197: \LMHASH{} `\LMHash{}`. https://codereview.chromium.org/2852533003/diff/60001/docs/language/dartLangS... docs/language/dartLangSpec.tex:7201: even if that library compiles successfully as a non-script library. If we keep the "`main(a,b,c){}` is detected at compile-time" approach, there is no need to describe something which is clearly a run-time thing, and then claiming that we can emit a compile-time error here. ;-)
What's the status on this?
https://codereview.chromium.org/2852533003/diff/60001/docs/language/dartLangS... File docs/language/dartLangSpec.tex (right): https://codereview.chromium.org/2852533003/diff/60001/docs/language/dartLangS... docs/language/dartLangSpec.tex:7164: that can be called with zero, one or two positional arguments. As discussed, I disagree that we should impose constraints on unrelated libraries by saying that all top-level functions called `main` must be script entry points. It's true that it might not help anybody because they won't be writing those, but it also enshrines the `main` function in the specification as special. If an embedder wants to have a different entry point, like the Windows "winmain" method which is different from "main", that's OK, and they can enforce whatever they want on their entry-point library, and they shouldn't affect other libraries. I hold ourselves to the same standard - the "main" function is not a *Dart language* feature, it's a tooling feature, it's the *standard* way to execute a Dart program, the hook that the embedder/compiler uses to start execution, but the *language* doesn't require it. Basically, the entire section should probably be considered non-normative. https://codereview.chromium.org/2852533003/diff/60001/docs/language/dartLangS... docs/language/dartLangSpec.tex:7170: Then, the top-level function defined by \code{main} Going for "top level function \code{main}". https://codereview.chromium.org/2852533003/diff/60001/docs/language/dartLangS... docs/language/dartLangSpec.tex:7172: with up to two arguments, depending on how many it will accept. Going for just "as follows:". The "up to two arguments" is specified in the following anyway. https://codereview.chromium.org/2852533003/diff/60001/docs/language/dartLangS... docs/language/dartLangSpec.tex:7183: it is invoked an object whose runtime type implements \code{List<String>} On 2017/05/10 09:30:34, eernst wrote: > `invoked [with] an object` Done. https://codereview.chromium.org/2852533003/diff/60001/docs/language/dartLangS... docs/language/dartLangSpec.tex:7190: the library is not considered a script. I don't plan to keep that approach, so retaining this comment. https://codereview.chromium.org/2852533003/diff/60001/docs/language/dartLangS... docs/language/dartLangSpec.tex:7197: \LMHASH{} On 2017/05/10 09:30:34, eernst wrote: > `\LMHash{}`. Done. https://codereview.chromium.org/2852533003/diff/60001/docs/language/dartLangS... docs/language/dartLangSpec.tex:7201: even if that library compiles successfully as a non-script library. As discussed, I disagree on that. It is not a runtime error to not be a script where a script is expected. Runtime starts when main is executed, and it isn't executed if it is not a script. So, it's either a compile-time error or just user error (like specifying an entry point that isn't a Dart file at all).
LGTM if 'As such, it should be reported as a compile-time error, even if that library compiles successfully as a non-script library' (line 7228 in the most recent patch set) is deleted, or it is modified to say that it is an implementation defined action to preclude said execution. https://codereview.chromium.org/2852533003/diff/60001/docs/language/dartLangS... File docs/language/dartLangSpec.tex (right): https://codereview.chromium.org/2852533003/diff/60001/docs/language/dartLangS... docs/language/dartLangSpec.tex:7164: that can be called with zero, one or two positional arguments. Unsurprisingly, I would still prefer to go from "static warning" to "compile-time error" rather than going to "no issues according to the spec!" (at least, separate compilation and hence the analyzer cannot do anything else). But being in the minority in the discussions IRL, I won't insist. https://codereview.chromium.org/2852533003/diff/60001/docs/language/dartLangS... docs/language/dartLangSpec.tex:7170: Then, the top-level function defined by \code{main} On 2017/05/30 05:45:38, Lasse Reichstein Nielsen wrote: > Going for "top level function \code{main}". Acknowledged. https://codereview.chromium.org/2852533003/diff/60001/docs/language/dartLangS... docs/language/dartLangSpec.tex:7172: with up to two arguments, depending on how many it will accept. On 2017/05/30 05:45:38, Lasse Reichstein Nielsen wrote: > Going for just "as follows:". > The "up to two arguments" is specified in the following anyway. Acknowledged. https://codereview.chromium.org/2852533003/diff/60001/docs/language/dartLangS... docs/language/dartLangSpec.tex:7190: the library is not considered a script. On 2017/05/30 05:45:37, Lasse Reichstein Nielsen wrote: > I don't plan to keep that approach, so retaining this comment. Right, that's the consistent approach. https://codereview.chromium.org/2852533003/diff/60001/docs/language/dartLangS... docs/language/dartLangSpec.tex:7201: even if that library compiles successfully as a non-script library. On 2017/05/30 05:45:38, Lasse Reichstein Nielsen wrote: > As discussed, I disagree on that. > > It is not a runtime error to not be a script where a script is expected. At this point I'm trying really hard to find a way to make this approach work well (that is, where `main(a,b,c) {}` is not an error, it's just the library which isn't a script). I assume that we do not want to add a section about linking. We also do not want to imply that separate compilation is a violation of the spec. This means that we just _cannot_ say that it is compile time when 'a non-script library is provided where a script is expected'. If a script is expected then we have a closed world, and we do not wish to require that the closed world is known during compilation. Please just drop lines 7200 and 7201. This means that it is up to the implementation to do something meaningful which will preclude that execution, and we do not specify what it is. > Runtime > starts when main is executed, and it isn't executed if it is not a script. > So, it's either a compile-time error or just user error (like specifying an > entry point that isn't a Dart file at all). User error is fine! And we don't specify that.
On 2017/05/30 09:14:42, eernst wrote: > ... if 'As such, it should be reported as a compile-time error, even if that > library compiles successfully as a non-script library' (line 7228 in the most > recent patch set) is deleted, or it is modified to say that it is an > implementation defined action to preclude said execution. If we don't specify what happens, we can't test it.
On 2017/05/30 09:44:18, ahe wrote: > If we don't specify what happens, we can't test it. That's not necessarily a problem. If it's not specified behavior, then we don't need to test it, we only need to test that we work on correct programs. Each tool can do whatever it wishes to give the user the best experience when using that tool. We can test that the program *doesn't run*, which is all that is actually required - it's not a "Dart program". The options are: - compile time error - runtime error - implementation dependent error A compile-time error precludes execution, so it matches well on that. When "compile time" happens is unspecified in Dart. Any time before running the `main` is definitely possible. If you expect a compiler to do *all* compilation of a program, then it should also report the non-script entry point. If you have separate compilation, then maybe you haven't specified an entry point, and so you have not compiled a *program*, just individual libraries. You need an extra step that links up the necessary separate modules and selects an entry point, and that step can report the compile-time error, or it can even be reported just before running the program. Runtime errors are something I prefer to reserve for actually throwing objects at runtime. There is noone to catch the error, and no context for a stack trace, before main runs, so I think it fits badly. Also, I definitely do not want dart2js to successfully compile a non-script and only fail at runtime. An implementation dependent error is basically a way to say "each tool can do what it wants". We can give a guide-line, but it's all about tool coherence, not language semantics, so it shouldn't be in the language specification. (I'm considering the entire section on scripts to be non-normative, it's just explaining how Dart is usually executed, but it doesn't say anything about the language itself). A complicating factor is Isolate.spawnUri, which we want to be consistent with running a program from scratch. Still, both a compile-time error and an implementation dependent error should be reported as a failure to spawn the isolate, so we can't really distinguish the two there. Effectively there is no visible user difference between a compilation error and an implementation dependent error, the end result is that the "program" won't run because it's not a script, and the error will be reported *at the latest* just before `main` would have been invoked. (On the other hand, errors from having an incorrect type are actually reported at runtime. We might want to say a little more about that, to make it clear that the runtime invocation of the `main` function includes the parameter binding, which is where type errors are reported in checked mode).
On 2017/05/30 10:32:15, Lasse Reichstein Nielsen wrote: > On 2017/05/30 09:44:18, ahe wrote: > > > If we don't specify what happens, we can't test it. > > That's not necessarily a problem. If it's not specified behavior, then we don't > need to test it, we only need to test that we work on correct programs. No. We need to test both correct and incorrect programs. > Each > tool can do whatever it wishes to give the user the best experience when using > that tool. That argument would make sense if you had people writing tools telling you please don't specify this. But that' not what's going on here. The tool makers are telling you that we need this specified so we can test it. We are telling you that we need your help resolving an issue that is preventing us from testing that our tools are working correctly. Please specify this behavior so that we can move on and fix other problems. > We can test that the program *doesn't run*, which is all that is actually > required - it's not a "Dart program". > > The options are: > - compile time error > - runtime error > - implementation dependent error > > A compile-time error precludes execution, so it matches well on that. When > "compile time" happens is unspecified in Dart. Any time before running the > `main` is definitely possible. > If you expect a compiler to do *all* compilation of a program, then it should > also report the non-script entry point. > If you have separate compilation, then maybe you haven't specified an entry > point, and so you have not compiled a *program*, just individual libraries. You > need an extra step that links up the necessary separate modules and selects an > entry point, and that step can report the compile-time error, or it can even be > reported just before running the program. > > Runtime errors are something I prefer to reserve for actually throwing objects > at runtime. > There is noone to catch the error, and no context for a stack trace, before main > runs, so I think it fits badly. Also, I definitely do not want dart2js to > successfully compile a non-script and only fail at runtime. > > An implementation dependent error is basically a way to say "each tool can do > what it wants". We can give a guide-line, but it's all about tool coherence, not > language semantics, so it shouldn't be in the language specification. (I'm > considering the entire section on scripts to be non-normative, it's just > explaining how Dart is usually executed, but it doesn't say anything about the > language itself). > > A complicating factor is Isolate.spawnUri, which we want to be consistent with > running a program from scratch. Still, both a compile-time error and an > implementation dependent error should be reported as a failure to spawn the > isolate, so we can't really distinguish the two there. > > > Effectively there is no visible user difference between a compilation error and > an implementation dependent error, the end result is that the "program" won't > run because it's not a script, and the error will be reported *at the latest* > just before `main` would have been invoked. > > (On the other hand, errors from having an incorrect type are actually reported > at runtime. We might want to say a little more about that, to make it clear that > the runtime invocation of the `main` function includes the parameter binding, > which is where type errors are reported in checked mode).
I already declared that I wouldn't insist --- but the situation would be simple and clear and testable etc. if we made it a compile-time error to have a top-level declaration named `main`, unless it is a function that can be invoked with at most 2 arguments. No exceptions, no doubt, no problem with separate compilation.
Description was changed from ========== Change specification of main. ========== to ========== Change specification of main. R=eernst@google.com, rnystrom@google.com Committed: https://github.com/dart-lang/sdk/commit/13f3df7e0cd4de9c99d344f242afb8514ada06f7 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) manually as 13f3df7e0cd4de9c99d344f242afb8514ada06f7 (presubmit successful).
Message was sent while issue was closed.
Thank you! |
