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

Allow reserved words in unquoted imports but disallow whitespace. #4035

Merged
merged 2 commits into from
Aug 21, 2024

Conversation

munificent
Copy link
Member

Fix #3983.
Fix #3984.

cc @dart-lang/language-team

Copy link
Member

@lrhn lrhn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.
Nits and comments only, core functionality is sound.
One suggestion: Don't allow packagePath after part or part of - I should probably file an issue.

working/unquoted-imports/feature-specification.md Outdated Show resolved Hide resolved
working/unquoted-imports/feature-specification.md Outdated Show resolved Hide resolved
working/unquoted-imports/feature-specification.md Outdated Show resolved Hide resolved
working/unquoted-imports/feature-specification.md Outdated Show resolved Hide resolved
```dart
import /* Weird but OK. */ some/path;
export some/path; // Hi there.
part some/path // Before the semicolon? Really?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Had a thought... (TL;DR: Maybe don't allow unquoted URIs for part directives.)

We should not allow unquoted URIs in part of declarations, because we havent' had time to migrate people off of part of library.name yet, and that would mean part of foo.bar; would go directly from one valid meaning to another different valid meaning.

The part of library.name won't be disallowed until "parts with import", or maybe this feature, whichever comes first. (Not unless we can remove it in a release before either, but that sounds unlikely. And it looks better to remove a feature as part of another feature that seem to bring value that compensates.)

I don't think it's a problem to not allow unquoted URIs in part of declarations, because the URI of part of is always going to be a relative path, and relative paths cannot be unquoted anyway. (A part must be in the same package as the file it is part of, "parts with imports" makes that explicit, but it was always at least implicitly assumed.)

With that reasoning, maybe we should also never use an unquoted URI in a part directive.
Should we choose to not allow it, in order to highlight that unquoted URIs are for (other) packages, local relative references should/must always be quoted paths, and part URIs should always be relative references?

(That a part is in the same package as its parent is not a language requirement for any particular language version, as much as it's a language versioning requirement, so our tools that accept multiple language versions require parts and parent files to be in the same package, so they have the same default language version. Most likely also require both to be inside lib/ or both outside of lib/, although that's more debatable. But if not, I can make the same part file part of two different libraries.)

At least unless/until we allow unquoted relative paths like ./foo or ../foo. But that's another feature 😁.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should probably be filed as an issue so that we don't lose the discussion. :)

For now, I'll leave the proposal as-is, but I'm definitely keen to talk about this more. For what it's worth, I would ultimately like to support unquoted parts. It's true that we don't support relative unquoted references. But my guess is that users will end up preferring unquoted package references even to files in the same package versus quoted relative imports. At least, that's what I think I'll end up doing.

If that's the case, then unquoted part directives will make sense too.

Copy link
Member

@lrhn lrhn Aug 21, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

preferring unquoted package references even to files in the same package versus quoted relative imports

I find it unlikely that most people will prefers part mypkg/src/subfeature/featurepart; to part "featurepart.dart"; in mypkg/src/subfeature/feature.dart.

We don't even have a way to omit the package name, and parts will always at least be below src/ too, and almost always in the same directory as the library file.

It also only works for libraries inside lib/. If you have part files in test/, bin/, tool/ or (now) hook/, you can't use unquoted package URIs for those.

But we'll see 😁 . (I expect users will ask for ways to not quote relative paths, and we can give them that, as part ./featurepart;. I still think we should use : to separate package name from path, because that makes it harder to typo a relative path into a package path. But the most likely mistake is to do import foo; intending it to mean import "./foo.dart";, which doesn't have any separator, so the separator probably won't make a big difference in anything. Maybe I just like the look of : better.)

it is looking at a `part` directive followed by an unquoted identifier like
`part of;` or `part of.some/other.thing;` versus a `part of` directive like
`part of thing;` or `part of 'uri.dart';` It must lookahead past the `of`
identifier to see if the next token is `;`, `.`, `/`, or another identifier.*
Copy link
Member

@lrhn lrhn Aug 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Requiring lookahead does mean we can't allow more words after part of.
Consider part of foo as f; which would be a part of the foo file, but which makes the imports inherited from foo available under f instead of in the top-level scope.
If we allow that, then part of deferred as f; would be ambiguous, if seen by itself. (EDIT: Well, no, it's a part, not an import, so deferred is not allowed on parts. We'd have to allow more on part directives too to get an ambiguity. /EDIT)

(In practice, a part file is almost always included by a parent file, so we know it's a part file. Only the analyzer and formatter sees files by themselves, and the formatter probably won't care. Also, why is this a part of package:deferred/deferred.dart?)

Or we could disallow packagePath for path directives. Shouldn't be a loss.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we allow that, then part of deferred as f; would be ambiguous, if seen by itself.

So you're saying this could be either:

  • A part of directive for a file named deferred which should be locally prefixed as f.
  • A part directive for a file named of which is deferred loaded and available under prefix f.

Is that right? If so, I don't think it's ambiguous since the latter isn't valid. We only allowed deferred on imports, not part, right? Does enhanced parts change that?

Copy link
Member

@lrhn lrhn Aug 21, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That was what I thought when I wrote it, but you can't have deferred parts (well, yet! ... but likely ever) so that's not really ambiguous.

So what I should have said is: If we allow something after part <uri> in the future, and that something can be a single identifier or dotted identifier, then part of something; is ambiguous.

And if we allow things after both part <uri> and part of <uri>, then if any thing after part <uri> has the form validUnquotedUri otherThing where otherThing is valid after part of <uri>, then part of *thing* is ambiguous.

A lot of "if"s.

The things I have actually considered so far are, during "parts with modifiers" discussions:

  • as id modifiers on part of and part to contain the identifiers they introduce.
  • sealed modifier on part of to just not inherit any imports from the parent file.

But, as Erik just pointed out to me, until we have a concrete ambiguity, we shouldn't be saying what an incorrect program means. That's a job for recovery and error messaging. We should say what allowed programs mean, and we should not allow programs that we don't want the user to write.

As currently specified, a valid program can have part id.id/id.id/id; with no spaces between the ids and the . and /s.
If it doesn't follow that format, it's not a valid Dart program, and we don't have to say how it's wrong, just that it is wrong.

That does mean that part of; is valid and means the same as part "package:of/of.dart";.
This is not parsing recovery if we allow the syntax and then give a "file/package not found" error. (It's incredibly unlikely that package:of/of.dart is a part file, and the current file is inside package:foo's lib directory, which is the only way it can not be an error.)

It's still surprising. It may be error-prone, if someone writes part of; and forgets to write a URI. Maybe they removed the library foo; because we tell them to, and then change part of foo; to just part of;. (It's obvious which file it's part of, right?)

If we want to prevent that, we can:

  • Not allow unquited URIs in part directives, or
  • Not consider a single identifier of as an unquoted URI in a part directive. (Basically the same as saying that of is a contextually reserved word, and contextually reserved words take precedence over everything else if they occur where we reserved them.)

Or we can do nothing, and have our tools recognize and give a tailored error message if someone does part of; and package:of/of.dart fails to exist or be a valid part of the current file. ("You wrote..., you probably meant to write ...".)

working/unquoted-imports/feature-specification.md Outdated Show resolved Hide resolved
@lrhn
Copy link
Member

lrhn commented Aug 15, 2024

Maybe add at least one example using conditional imports:

import smarts/basic
  if (dart.library.io) smarts/native
  if (dart.library.js_interop) smarts/js;

@munificent munificent merged commit 94194ce into main Aug 21, 2024
2 of 3 checks passed
@munificent munificent deleted the revise-unquoted-imports branch August 21, 2024 00:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

URI shorthands, allow reserved words. URI shorthands without internal whitespace.
3 participants