Skip to content

Minor hashbang-related inconsistency between the analyzer & spec and the compiler. #52331

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
modulovalue opened this issue May 10, 2023 · 9 comments
Labels
implementation Track the implementation of a specific feature (use on area-meta issue, not issues for each tool) legacy-area-front-end Legacy: Use area-dart-model instead.

Comments

@modulovalue
Copy link
Contributor

Consider the following two files:

// This is a single file called 'foo.dart'.
part 'foo.gen.dart';

void main() {
  run();
}

and

#! foo
// This is a single file called 'foo.gen.dart'.
part of 'foo.dart';

void run() {
  print("foofoo");
}

The analyzer emits an error on the keyword part in foo.gen.dart that says:

Directives must appear before any declarations.

Try moving the directive before any declarations.

This seems to suggest that foo.dart should not compile successfully. However, we can run foo.dart using dart foo.dart and it terminates successfully and prints foofoo.

According to the spec, the analyzer exhibits correct behavior and foo.dart should not compile because the grammar does not imply that a partHeader with a SCRIPT_TAG is in the Dart language.


I'm not too familiar with how the shell treats hashbangs and I might be missing some details, but, in my opinion, it looks like the way the compiler behaves allows for a cleaner separation of concerns and It might make sense to keep its behavior and adjust the analyzer and the spec:

utfSublanguage ::= FEFF? shellSublanguage
shellSublanguage ::= SCRIPT_TAG? dartSublanguage
dartSublanguage ::= ...
...

because it would allow us to factor out the shell-related parts out of the core dart language grammar instead of only allowing libraryDefinitions to be members of the "shellSublanguage".

@lrhn
Copy link
Member

lrhn commented May 10, 2023

Yep, I agree that the analyzer is correct. A part file cannot start with #!.

Most likely the compilers never check, they just treat all initial #! lines as comments, and ignore them completely.

I don't see any particular value in making it an error, so maybe we should allow part files to start with a script tag too.
@eernstg

@lrhn lrhn added the legacy-area-front-end Legacy: Use area-dart-model instead. label May 10, 2023
@eernstg
Copy link
Member

eernstg commented May 10, 2023

I don't see any particular value in making it an error, so maybe we should allow part files to start with a script tag too.

We could do that.

Just testing, it is actually possible to run a program which is determined by a part file: The part file is part of some library; that library is determined to the same extent as any other Dart source file if the part uses a URI to denote it; and dart myPart.dart will run the program when myPart.dart specifies a library which can be the entry point for a Dart program execution (that is, it exports a top-level function named main, cf. this spec paragraph).

In other words, putting a script tag into a part makes sense for the exact same reason that it makes sense to put it into a library, and it could even be used to run the same dart program in several different ways, based on different script tags in different parts. ;-)

@lrhn
Copy link
Member

lrhn commented May 10, 2023

Being able to use a part file as entry point sounds like being outside of the specified behavior. Unless we've been very vague in specifying, which never happens!

Even then, the #! shell trigger can be on any file, it doesn't have to start running the specified executable with that file, it's just easier to do that.

So we could allow a part file to start with a script tag.
Or we could make the front end err if it sees a part file with a script tag.
Or we can close our eyes and keep pretending nobody ever noticed.

In the end, I didn't it'll ever matter.

@modulovalue
Copy link
Contributor Author

I agree that this issue is not urgent and doesn't seem to matter much today, but I still think that we should be clear about what is considered to be correct behavior here so that the spec, analyzer, compiler and other tools can behave in a consistent manner.

Being inconsistent here can have unwanted effects like the following:

Bildschirm­foto 2023-05-11 um 01 11 31

Notice how the warning says "Try moving the language version override to the top of the file.". A literal interpretation of that proposed solution would introduce a new error.

As humans we can understand what's happening here and resolve the ambiguity on the spot, but if we were to try to offer automatic fixes for this warning, then we would realize that the spec & analyzer and the compiler do not agree on what "the top of the file" refers to here.

@johnniwinther
Copy link
Member

The ability to run a program from a part is accidental and only works if the part of declaration refers to the URI and not just the library name. This is an inadvertent consequence of supporting seeing part files before their containing library during incremental compilation. I'd very much like not to have to support this continuously since it is part of a long standing technical debt within the CFE.

@eernstg
Copy link
Member

eernstg commented May 11, 2023

Being able to use a part file as entry point sounds like being outside of the specified behavior.

It's implementation specific behavior, and the language specification in general does not say anything about that. But we do have this commentary in the section 'Scripts', which makes execution of parts 'atypical', if anything:

A Dart program will typically be executed by executing a script.

However, as @johnniwinther just mentioned, the ability to execute a part relies on using a specific kind of part of directive, which makes this ability half-done anyway. I'd be quite happy if we just keep the current grammar (such that a script tag can occur in a library, but not in a part).

On top of the syntax error for the script tag in a part, each compiler/runtime can report an error if an attempt is made to execute a part (which is possible also in the case where there is no script tag anywhere).

@eernstg
Copy link
Member

eernstg commented May 17, 2023

@johnniwinther, considering that the analyzer rejects a part with a script tag, it should be safe to make it an error with the CFE as well.

In that case I'd suggest that we put the label 'implementation' on this issue and use it to be the issue that requests an implementation of this error in the CFE at some point in time where it fits the priorities. WDYT?

@johnniwinther
Copy link
Member

SGTM

@johnniwinther johnniwinther added the implementation Track the implementation of a specific feature (use on area-meta issue, not issues for each tool) label May 22, 2023
@eernstg
Copy link
Member

eernstg commented Jul 21, 2023

@eernstg eernstg closed this as completed Jul 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
implementation Track the implementation of a specific feature (use on area-meta issue, not issues for each tool) legacy-area-front-end Legacy: Use area-dart-model instead.
Projects
None yet
Development

No branches or pull requests

4 participants