-
Notifications
You must be signed in to change notification settings - Fork 213
Specify interactions with older language versions #2538
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
Conversation
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.
LGTM!
accepted/future-releases/records/records-feature-specification.md
Outdated
Show resolved
Hide resolved
the language version in which records are released, the following errors apply. | ||
|
||
It is an error to reference the SDK class `Record` in a library whose language | ||
version is less than `v`. |
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.
You say below:
It is not an error for a library whose language version is less than
v
(a
"legacy library") to include or reference theRecord
class, record types or
record expressions directly or indirectly from another library whose language
version is greater than or equal tov
.
That directly contradicts this paragraph (dart:core
is a library with language version >= v).
Just drop this paragraph entirely, it's not a restriction we need.
The Record
declaration will be marked with @Since("v")
, so we can still give warnings if you use it in a package with min-SDK < v.
(And if your min SDK >= v anyway, there is no problem, even if the referencing library is held back by a //@dart=<v
marker.)
Also "reference" is a vague word - as proven by it being used in the quoted paragraph too, with a seemingly different meaning.
Going through a typedef is still "referencing" to me (and to the quoted paragraph, as I read it). To disallow direct references to the Record
declaration, we'd have to be explicit and, say, state that you are not allowed to have the identifier Record
in a place where it resolves to/denotes the Record
class declaration of dart:core
.
Just drop this restriction.
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.
Just drop this paragraph entirely, it's not a restriction we need.
I don't think this is true. I can clarify this, I see no issues with specifying this more precisely if you feel that the intent is unclear, but this is an important error. Without this error, you can write a library which uses the Record
class (which will work with no errors or warnings, since you are running on an sdk that supports it) and then ship the code to pub without raising your sdk constraint, whereupon you break the world.
The Record declaration will be marked with
@Since("v")
, so we can still give warnings if you use it in a package with min-SDK < v.
(And if your min SDK >= v anyway, there is no problem, even if the referencing library is held back by a//@dart=<v
marker.)
Empirically, the @Since
annotation doesn't seem to work this way. I can write a package with sdk constraint sdk: ">=2.9.0 <3.0.0"
and use the Object.hash
method, which is marked @Since("2.14")
with no errors or warnings when running with a bleeding edge SDK.
To disallow direct references to the Record declaration, we'd have to be explicit and, say, state that you are not allowed to have the identifier Record in a place where it resolves to/denotes the Record class declaration of dart:core.
This is more restrictive than we need (it's fine to use the identifier Record
, which resolves to the Record
class from dart:core
, if it is re-exported via another library). But I don't know that we care about this use case. I'll update this with more precise wording, either using this restriction or something more fine grained.
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 expecting to not restrict access to the type. We don't generally restrict access to new features in the platform libraries to libraries with language versions before the SDK where the feature was introduced.
We could, and then it should really apply to every @Since
-annotated platform declaration, but so far we haven't done that. We didn't do anything for the Enum
class (2.14) or the enum name extension (2.15). So, is there something which makes Record
special in this regard, or is it something we would want to apply more generally in the future?
As for phrasing, if we do want the restriction, maybe:
... must not contain the identifier
Record
in a place where the name denotes theRecord
class declaration ofdart:core
, and where that import scope name is only imported from platform libraries.
That should allow any reference which is also exported through a non-platform library.
(And yes, the analyzer completely ignores @Since
annotations. I still hope we'll eventually start recognizing them.)
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 expecting to not restrict access to the type. We don't generally restrict access to new features in the platform libraries to libraries with language versions before the SDK where the feature was introduced.
Record
isn't really a platform library feature though. It's a language feature, and is built into the semantics of the language. The fact that the implementations choose to pretend that there's a class named Record
in the platform libraries gives us a nice enough place to hang documentation and such off of, but the type can't actually be implemented that way (it has an unbounded set of subtypes, which are not class types, for one; it's sealed, for another; there are also sorts of special rules in the meta-theory that refer to it, for a third).
So it feels odd to me to treat it differently from the other record types, just because we happen to have decided to pretend that there is a class named Record
from which the type is derived.
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.
... must not contain the identifier Record in a place where the name denotes the Record class declaration of dart:core, and where that import scope name is only imported from platform libraries.
I used something modeled after this.
syntactically in a library whose language version is less than `v`. | ||
|
||
It is an error for the record type syntax (e.g. `(int, int)`) to be used | ||
syntactically in a library whose language version is less than `v`. |
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.
If we are specifically enumerating the differences (and not just implicitly saying that the older language versions use the language which was current at that point), we should also mention the disambiguation changes:
try { } catch { } on (e) {}
has a method declaration namedon
in language < v, but doesn't in language >= v.@Foo () Function() f;
is a constructor invocation in the annotation in language < v, but isn't in language >= v.
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 think this should be covered elsewhere (e.g. by the section that @munificent is adding on the grammar). This section describes the interactions between different language versions: it does not specify how the syntax of the the current language version is different from the syntax of the old language version.
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.
Not sure I can see the distinction.
Saying that
It is an error for the record type syntax (e.g.
(int, int)
) to be used
syntactically in a library whose language version is less thanv
.
does look like it's saying where the new syntax differs from the old syntax - this syntax is in the new language, and not in the old.
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.
The point of this section is to describe the interactions between language versions. It does so by circumscribing the set of errors that arise from using record features in older language versioned libraries (and noting that certain things are not in that set of errors, e.g. types that arise by inference). The first of the two things you mention is already covered in this proposal, I don't see value in adding it here. The second thing, I don't know the answer to, it's still under discussion.
If you really believe that these should be covered here, I'd suggest following up with a PR to re-organize as you prefer.
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.
ACK. If this section is simply on records, and not the entire feature around them (and we don't even know if those two changes will be rolled into the records flag, or just be released independently), then it makes sense to focus only on the actual record types.
I was considering those two changes as part of the records feature, at an equal level to record syntax, but that's probably premature.
So keep as is.
Specify the errors for when record syntax is used in libraries with an older language version, and clarify the treatment of record types and values that flow into libraries on older language versions.