dart-lang
Annotations on record fields are challenging to implement; can we drop them?
The current records spec has this grammar for record types:
recordType ::= '(' recordTypeFields ',' recordTypeNamedFields ')'
| '(' recordTypeFields ','? ')'
| '(' recordTypeNamedFields? ')'
recordTypeFields ::= recordTypeField ( ',' recordTypeField )*
recordTypeField ::= metadata type identifier?
recordTypeNamedFields ::= '{' recordTypeNamedField
( ',' recordTypeNamedField )* ','? '}'
recordTypeNamedField ::= type identifier
recordTypeNamedField ::= metadata typedIdentifier
This means, in particular, that it's valid for a record field to have an annotation like @deprecated:
(String, {@deprecated int n}) foo() => ('s', n: 1);
It seems reasonable that a user might want to write this, and expect that it would cause a deprecation hint to be printed here:
test() {
print(foo().n); // HINT: don't access `.n`; it's deprecated.
}
But this is complicated to implement because record types are structural, meaning that anywhere the type (String, {int n}) appears in the program, it always denotes the same type. How should the analyzer behave if some instances of (String, {int n}) have a @deprecated annotation on int n and some don't? For instance, what if the test code had been this instead?
test2() {
(String, {int n}) x = foo();
print(x.n);
}
Now, under one possible intrepretation of the spec, x has type (String, {int n}) (with no annotations), and the assignment x = foo() is allowed because (String, {int n}) and (String, {@deprecated int n}) are the same type. But that means that print(x.n) won't show a hint anymore. That seems bad.
Possible solutions:
-
We could say that if the field
nis deprecated in one place, it's implicitly deprecated in all other equivalent record types, or all other record types with the same shape. But this seems like it will lead to false positivies, because the program could have many other instances of(String, {int n})that are unrelated, and they shouldn't all lead to warnings just because one of them does. -
We could say that if
(String, {@deprecated int n})and(String, {int n})are actually different types, but this seems bad because it means that adding@deprecatedto a function's return type is now a breaking change. (E.g.test2()above would no longer compile). And it defeats the purpose of@deprecatedif you have to do a major version number bump on your package in order to use it. -
We could say that the analyzer has to do a "best effort" job of propagating annotations like
@deprecatedthrough the various "equivalent" record types. In order to be actually useful, this would be decidedly non-trivial. At a minimum, it would probably need some sort of dataflow analysis similar to the existing "flow analysis" we have for nullabilities, and flow analysis was a large undertaking. And even that would need to be augmented by whole-program analysis if we wanted it to catch more than the most trivial cases. No matter how we do it, anything short of symbolic execution would result in false negatives, which might eventually lead people to distrust the feature.
Or, the fourth option, which I'm proposing, is:
- Don't allow annotations on record fields at all. Implementation-wise, this is the cheapest option, since all it requires is removing a feature that nobody is using yet. In terms of the effect on the user, it does mean they can't use
@deprecatedin certain places where they otherwise could, but on the plus side, we'll be avoiding a bunch of false-negative or false-positive bugs that might erode the user's confidence in annotations like@deprecated.
Note that a similar situation arises today with inline function types. The analyzer folks believe that annotations on their arguments get pretty much dropped on the floor. But I ran a test case and I found that with this input the analyzer actually crashes:
foo() {
void Function({@deprecated int i}) b = bar;
print(b);
b(i: 1);
}
bar({@deprecated int i = 0}) {}
(Note that the crash isn't trivial to reproduce because dart analyze hides the crash. See https://github.com/dart-lang/sdk/issues/49931).
If we decide to drop support for annotations on record type fields, we should also consider dropping support for annotations on inline function types in the Dart 3.0 release.
CC @munificent @leafpetersen @kallentu @natebosch @jakemac53 @lrhn @mit-mit @eernstg @scheglov @bwilkerson @srawlins
I'm in favor of disallowing metadata here unless we get a concrete use case for it. I agree that the semantics of deprecating a field in a record type don't make sense.
I would be very curious to see any practical use cases for metadata in function types today.
I have no problem not allowing it. The main use case I can imagine is for some tool that wants to generate or rewrite Dart code based on markers in the input code. I don't know of any actual current uses though.
If we have no solid use cases, let's disallow it. We can always add support later.
I think we will allow it later. The general direction for metadata has been to allow it in more and more places, because people might want to say something about something in order to affect, say, lints or warnings.
Consider having an annotation that makes a future not trigger the unawaited_future lint (say @unimportant).
Then you may want to write things like:
(@doNotStore Future<String> result, @unimportant Future<bool> sideChannel) compute() { ... }
I don't think the grammar for annotations in records is any worse than than annotations in parameter lists. (The grammar problems we have are independent of this).
I'm fine with not allowing it now, if we want the dev-hours for something else, but I fully expect to get a request for it very soon after launch.
The problem with annotations is that they are at source concept. It matters where they occur in the source, whether that corresponds to a static or runtime concept. That means that you can meaningfully put annotations anywhere in the source code, as long as some tool can read and understand them. For types where you can go from reference to declaration, you can use that to get to annotations on the declaration source, but that doesn't mean that structural type literals cannot be annotated anyway. (And for a declaration like (@foo int, @bar int) foo();, you can go from the reference to foo to the syntax of its return type.
Is it possible to catch the @deprecated when the value is assigned?
test2() {
(String, {int n}) x = foo(); // HINT: `.n` is deprecated, consider removing from the record or annotating with @deprecated
print(x.n);
}
Essentially making (String, {@deprecated int n}) and (String, {int n}) different types, but allowing implicit casts between them (like from int to num)
This would allow a library to update its code without any breaking changes, and deprecated would be warned about either when accessing directly like in case 1, or when "removing" the deprecation from the type like in case 2.
In case this affects anyone's calculus of the value: are there any @pragma annotations that the VM or dart2js might use on a record type field?
Is it possible to catch the @deprecated when the value is assigned?
test2() { (String, {int n}) x = foo(); // HINT: `.n` is deprecated, consider removing from the record or annotating with @deprecated print(x.n); }Essentially making
(String, {@deprecated int n})and(String, {int n})different types, but allowing implicit casts between them (like from int to num)This would allow a library to update its code without any breaking changes, and deprecated would be warned about either when accessing directly like in case 1, or when "removing" the deprecation from the type like in case 2.
AFAIK you cannot change (String, {int n}) x = foo(); to (String, ) x = foo();, because there is no "row polymorphism", as described in the specification. So, it is either the exact type annotation, or var. If it is not var, the code will definitely break. if foo() changes its return type.
Making two types different based on whether there are annotations is weird, we have not done anything like this yet.
I'm curious about what the analyzer is currently doing for function types. Given the following example:
void foo({@deprecated int x = 3}) {}
void main() {
foo(x : 3);
var f = foo;
f(x : 3);
void Function({int x}) g = foo;
g(x : 3);
}
I see deprecation warnings on the first and third lines of main, which suggests that the analyzer is doing some propagation here. I wonder if this gives a guideline for how to treat records?
The problem with annotations is that they are at source concept. It matters where they occur in the source, whether that corresponds to a static or runtime concept. That means that you can meaningfully put annotations anywhere in the source code, as long as some tool can read and understand them. For types where you can go from reference to declaration, you can use that to get to annotations on the declaration source, but that doesn't mean that structural type literals cannot be annotated anyway. (And for a declaration like
(@foo int, @bar int) foo();, you can go from the reference tofooto the syntax of its return type.
Do you think we can go to the declaration in this case?
final x = <(@deprecated int, String)>[];
Or here?
final x = [0].map((e) => <(@deprecated int, String)>[]);
final y = x.first[0].$0;
The issue is basically that if annotations are not in fixed positions in AST, which we model as elements in the analyzer, and so can attach annotations to them, it is hard to find a good place to attach them to.
I'm curious about what the analyzer is currently doing for function types.
In your example the annotation is being handled because it's on a function declaration's parameter. But when the annotation is on the parameter of a function type, it isn't handled. For example:
void f(void Function({@deprecated int x}) g) {
g(x : 3); // No warning
}
Is the issue here that the analyzer model for a declaration has a "type" for the return type, which is a semantic entity, not a representation of the return type source code? Which means that even if the source contains annotations, the analyzer model cannot represent that.
If so, could the solution be to somehow attach metadata on the side, say a "source path" + annotation declaration on the side of a declaration, where someone interested can dig down and see if a particular part of the type was annotated in the source code?
In your example the annotation is being handled because it's on a function declaration's parameter. But when the annotation is on the parameter of a function type, it isn't handled. For example:
Does this generalize to functions with record return type then? That is, I wonder whether in the same way that you don't keep metadata on parameters of function types, but you do keep it on parameters of function declarations, you could keep metadata around for return types of function declarations (but not return types of function types).
Is the issue here that the analyzer model for a declaration has a "type" for the return type, which is a semantic entity, not a representation of the return type source code? Which means that even if the source contains annotations, the analyzer model cannot represent that.
Yes, a type is a semantic entity. Not all RecordTypes or FunctionTypes have source code presentation (in the contrast all InterfaceType have one). For example, there is no source code for the type of x below:
int f1() => throw 0;
double f2() => throw 0;
const b = true;
const x = b ? f1 : f2;
If so, could the solution be to somehow attach metadata on the side, say a "source path" + annotation declaration on the side of a declaration, where someone interested can dig down and see if a particular part of the type was annotated in the source code?
I don't understand this portion.
I'm not firmly attached to the idea of annotations on record fields but I agree with Lasse that users probably will ask for them, especially once macros are a thing. I'm fine if they aren't propagated around and they definitely aren't part of the static type. Annotations are a metadata property of a syntactic element in the source code. They don't annotate the type, they annotate a piece of syntax that may happen to produce a type.
This probably limits how @deprecated can work on record fields, but that's fine with me.
... they definitely aren't part of the static type.
Excellent!
Annotations are a metadata property of a syntactic element in the source code.
I think that's where my understanding breaks down. If I have the following code:
@deprecated
void f() {}
void g() {
f();
}
Then the invocation of f on line 5 is known to refer to the declaration of f on line 2, and because that declaration is annotated as being deprecated analyzer can flag the invocation.
But compare that to the following code:
void f(({int x, @deprecated int y})? a, ({int x, int y}) b) {
var r = a ?? b;
r.y;
}
What does the access of y on line 3 refer to? Does it refer to the field y on the parameter a, or the field y on the parameter b? Is it annotated as being deprecated or not?
I just don't know what it means to have an annotation on a field (or parameter, for that matter) of a structural type.
I just don't know what it means to have an annotation on a field (or parameter, for that matter) of a structural type.
I don't think it means anything. But it seems like the analyzer does support having annotations on parameters of declarations. And I don't really understand why that's different from fields of declarations. Examples:
Currently I see the following analyzer behavior:
void foo({@deprecated int x = 3}) {}
@deprecated
void bar() {}
void main() {
{
foo(x : 3); // warning
var f = foo;
f(x : 3); // warning
void Function({int x}) g = foo;
g(x : 3); // no warning
}
{
bar(); // warning
var f = bar;
f(); // no warning
void Function() g = bar;
g(); // no warning
}
}
I don't really understand the warning on f(x : 3); // warning (since that seems to suggest that the deprecation flows with the type), but otherwise this all makes sense. When I reference a specific declaration (bar, or the parameter x of foo), I get a deprecation warning, but if I reference something which was derived from bar or foo via control flow, I do not (except, again, the odd case of f(x : 3) which I don't know how to think about.
Extrapolating from this, I would (naively) expect the following behavior:
({@deprecated int z}) baz() => (z : 3);
void main() {
{
baz().z; // warning
var f = baz;
f().z; // no warning
({int z}) Function() g = baz;
g().z; // no warning
}
}
Does that make any sense? I don't claim that this model matches up with how annotations work in practice, and it's possible that even in theory I'm off base here, but naively this seems like the natural correspondence, and I think is what @munificent and @lrhn are suggesting (correct me if I'm wrong).
Extrapolating from this, I would (naively) expect the following behavior:
({@deprecated int z}) baz() => (z : 3); void main() { { baz().z; // warning var f = baz; f().z; // no warning ({int z}) Function() g = baz; g().z; // no warning } }
On further thought, I'm not 100% sure I agree with myself here. The direct analogy, I think, is more like this:
({@deprecated int z}) r = (z : 3);
void main() {
{
r.z; // warning
var f = r;
f.z; // no warning
({int z}) g = r;
g.z; // no warning
}
}
In the example I have above with baz, the reference to the deprecated field is through an expression, not directly through a reference to a declaration, and so arguably there should be no warnings on any of those uses?
But it seems like the analyzer does support having annotations on parameters of declarations.
Yes, but it's only a best effort sort of support rather than a well defined semantics that we're implementing.
I don't really understand the warning on f(x : 3); // warning (since that seems to suggest that the deprecation flows with the type) ...
That's more of an accident in the implementation than anything intentional. In the analyzer we associate the annotation with the Element declared by the parameter x, we remember that that parameter is part of the function foo, and we associate the Element for the function foo with the function type for foo.
For the invocation of foo(x : 3), we figure out that it's an invocation of foo, use the Element for foo to find the Element for x, see the annotation, and report a diagnostic.
For the invocation of f(x : 3) we infer that f has the same type as foo so we use the same instance of DartType used to represent the type of foo. That instance has a reference back to the Element for foo, so we accidentally carry forward the annotation. In other words, it's an accident caused by sharing a reference to an instance of DartType.
If that's not the desired behavior, then we have a bug.
But it seems like the analyzer does support having annotations on parameters of declarations. And I don't really understand why that's different from fields of declarations.
The difference (which might be purely wrong thinking on my part) is that it seems like record types don't have a unique declaration; they have one declaration per syntactic occurrence.
As a result, it doesn't appear to fit well with our existing model of elements and types, which is what led to this issue being opened.
I think the right path forward is to start by defining the semantics that we want to implement (hopefully the same semantics that users would like to see) and then figure out how to model those semantics, rather than by trying to see what we can and can't do with the current implementation.
If that's not the desired behavior, then we have a bug.
Stepping back, I think I would say that it's up to you to decide whether or not you have a bug. That is, as best I know, the language doesn't define how annotations are accessed or used. It just says "you can have an annotation here". And then various libraries (e.g. package:analyzer) provide APIs for querying the AST for the presence or absence of those annotations. How you represent and connect them up is not, as best I know, something that we try to specify in the language (though arguably at one point with dart:mirrors it was de facto part of the language).
I agree that there is no official, correct treatment of annotations. The only thing that matters is whether it's syntactically allowed and practically useful.
The underlying model, including what dart:mirrors did, has so far seemed to be that annotations belong to declarations (something which can be named and denoted), and tools then define how information flows from the declaration annotations to places where the annotation matters. Each annotation can do that in their own way.
The @deprecated annotation is a good example because it has a fairly clear underlying interpretation: This annotated thing will go away in the future. You should get a deprecated-use warning if your code would break when it goes away.
The ({int x, @deprecated int z}) foo() =>...; method is interesting because it's both clear what it's saying (it returns a record, and the record's z field will go away in the future), but it's also not easy to make actionable.
You can (maybe) write your pattern matches as foo() case (: var x, ...) to match both before and after the change, but the two types are unrelated.
It still comes back to when to give a warning, and that should really just be "best effort". It's not something that we have defined thoroughly, it's more like "if you can see this code will break when the deprecated thing goes away, warn. If you can't, shucks." Basically, it all depends on which analysis is used to create the warnings, and we don't specify that. It's always valid to make the analysis more precise, all it does is to emit warnings for programs which actually have a problem. (And "unless the code is in the same library/class/method/something which introduced the deprecated thing, so it probably knows already". A method using its own deprecated parameter shouldn't cause a warning. That's exactly the kind of context- and user-behavior-awareness that we want the analyzer to be able to make decisions about. Don't give a warning if it's not going to help the user anyway.)
The annotation is talking about the behavior of the function returning the tuple, it's attached to the function declaration, through its signature, not to the tuple type itself.
({@deprecated int z}) r = (z : 3);
void main() {
r.z; // Possible warning
var f = r;
f.z; // Possible warning.
({int z}) g = r; // Possible warning. The assignment won't stay valid when `z` is removed.
g.z; // Probably no warning, also because it would be redundant with the one above.
}
We don't require any of these warnings. We'd probably expect at least the first one. The rest would be best-effort only, but as long as there are no false positives, anything you do is fine.
The difference (which might be purely wrong thinking on my part) is that it seems like record types don't have a unique declaration; they have one declaration per syntactic occurrence.
That's correct. Like function types, they're structural. Two unrelated places in the program may annotate record types with the same fields and those two annotations denote the exact same static type.
As a result, it doesn't appear to fit well with our existing model of elements and types, which is what led to this issue being opened.
Yup. (I seem to recall similar friction when the int Function(String) function type syntax was added. Prior to that, the only place you could use function types was in function-typed parameters and typedefs.)
I think the right path forward is to start by defining the semantics that we want to implement (hopefully the same semantics that users would like to see) and then figure out how to model those semantics, rather than by trying to see what we can and can't do with the current implementation.
Sounds good to me, but I'll stress that those semantics are out of the hands of the language. The language doesn't ascribe any semantics to metadata annotations, nor does it associate them with types. They're just a piece of syntax you're allowed to put on some pieces of syntax. From the language's perspective, aside from grammatical restrictions, they're morally comments. If analyzer wants to define @deprecated and use it in various ways, that's all OK by me and by the language. If it doesn't make sense for analyzer to handle @deprecated on a record field, it's entirely reasonable for analyzer to report a warning if you use that metadata annotation on record fields.
I'm going to close this because I don't think we plan to change the proposal. I believe users will want metadata annotations on record fields once macros come around, and I think it's OK to support them.