Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Allow reserved words in unquoted imports but disallow whitespace. #4035
Changes from 1 commit
98652a6
bf69bf1
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
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 ofpart of library.name
yet, and that would meanpart 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 ofpart 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 oflib/
, 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 😁.There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I find it unlikely that most people will prefers
part mypkg/src/subfeature/featurepart;
topart "featurepart.dart";
inmypkg/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 intest/
,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 doimport foo;
intending it to meanimport "./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.)There was a problem hiding this comment.
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 thefoo
file, but which makes the imports inherited fromfoo
available underf
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, sodeferred
is not allowed onparts
. We'd have to allow more onpart
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
forpath
directives. Shouldn't be a loss.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So you're saying this could be either:
part of
directive for a file nameddeferred
which should be locally prefixed asf
.part
directive for a file namedof
which is deferred loaded and available under prefixf
.Is that right? If so, I don't think it's ambiguous since the latter isn't valid. We only allowed
deferred
on imports, notpart
, right? Does enhanced parts change that?There was a problem hiding this comment.
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, thenpart of something;
is ambiguous.And if we allow things after both
part <uri>
andpart of <uri>
, then if any thing afterpart <uri>
has the formvalidUnquotedUri otherThing
whereotherThing
is valid afterpart of <uri>
, thenpart 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 onpart of
andpart
to contain the identifiers they introduce.sealed
modifier onpart 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 theid
s 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 aspart "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 insidepackage:foo
'slib
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 thelibrary foo;
because we tell them to, and then changepart of foo;
to justpart of;
. (It's obvious which file it's part of, right?)If we want to prevent that, we can:
part
directives, orof
as an unquoted URI in apart
directive. (Basically the same as saying thatof
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;
andpackage:of/of.dart
fails to exist or be a valid part of the current file. ("You wrote..., you probably meant to write ...".)