Macros: augmenting a private final field with a getter creates a chicken-and-egg problem

[stereotype441]

The current rules for field promotion say that a getter is promotable as long as the name of the getter is private to the library, and the library doesn't contain one of the following things:

• a concrete getter with the same name,
• a non-final field with the same name,
• an external field with the same name, or
• a concrete class that contains a getter with the same name in its interface but not in its implementation (such a class is either erroneous, or implicitly forwards that getter to noSuchMethod).

This means that if a macro were to augment a private final field with a getter, then that field (and other private final fields with the same name in the same library) might be considered promotable before the augmentation, but not promotable after it.

That's a problem because during phase 3, a macro implementation is allowed to query the inferred type of an OmittedTypeAnnotation through the builder APIs. But if the type of a field or top level variable needs to be inferred from its initializer, then that initializer must be analyzed, and its type can depend on whether a field is promotable. This creates a chicken-and-egg problem: the inferred type is needed by the macro, but it can't be correctly computed until all macros have completed.

It's tempting to solve this chicken-and-egg problem by allowing the builder API to query an OmittedTypeAnnotation to block and invoke other macros, and to throw a MacroIntrospectionCycleException if there's a dependency cycle (much as we currently do in phase 2), but it would be challenging, because (a) it would require knowing ahead of time the names of the fields that a phase 3 macro might augment (and I don't think that's currently possible), and (b) it would require rewriting the logic for field promotion so that it can operate one name at a time (rather than one library at a time, as it currently does).

I think the better approach would be to prohibit final fields from being augmented by a getter.

@dart-lang/language-team FYI

Edit: as pointed out below, it would also be a problem if an abstract getter were augmented by a concrete getter. So we would conceivably have to prohibit that as well.

[lrhn]

I'm starting to wonder if the only way to be safe is to reanalyze the entire program (or as much as needed) after each macro phase.

The types, and other derivable properties, that you see before a phase are tentative, because we cannot assign definite types to a partial Dart program, and anything which is changed by macro code additions was by definition only a partial program.

Macros are part of the library. If a macro changes an instance variable to be augmented with a getter, then that variable is not promotable. It never was, because it's always been a getter, there is no such thing as the library without the macro applied. That's at most an intermediate step during compilation, but it's not the program.
Which brings us back to what we can tell the macro code about the intermediate program, which is not necessarily a (complete) Dart program.

So for this concrete problem: The step 3 macro sees the type that the step 2 macro left it with. If that's not the same type as what step 3 leaves the program with, that's just what it is. If that leads to a compilation error, write better macros. Or don't mix incompatible macros.

[jakemac53]

All we should have to do here is disallow augmenting final field getters, which it is anyways pretty reasonable to disallow. It is a very unexpected thing for a field which is declared final, to have a getter which does not return the value of its backing store, or worse an unstable value.

If we get stable getters, we could discuss allowing augmenting a final field getter with a stable getter, which wouldn't break promotability at least, but would still be pretty weird.

[lrhn]

I this a problem for plain getters too?
If a private getter is declared as abstract, and then a body is added in macro step 3, then that should also change another final variable of the same name from being promotable to not.
(That's an issue we've seen before - the inability to distinguish an abstract declaration from an "eventually given implementation" declaration until the implementation is added.)

At least it's not possible to augment an abstract getter into a variable.

[davidmorgan]

IIRC from the stable getters work, what people usually want/mean when they write a final field is that it is stable, even if there is no such concept in the language.

However it is perfectly reasonable to implement that as a getter, for example one that does caching.

I would even say that a macro that does so is a particularly appropriate use of a macro: if you design your code so it's always a human writing the getter implementation, they might accidentally write an unstable getter; but if you use a macro you can easily get it right and have it generate only stable getters, no help needed from the language.

So an alternative approach would be to leave it up to the augmentation/macro author and say: if the language decided this field is promotable, and you augmented it with an unstable getter, you get undefined behaviour--the implementation fallout is on you.

It doesn't feel particularly different to other design/implementation issues macro authors face: it's always about creating a regular structure which gives useful guarantees on top of whatever manual code might get thrown at you. So it's anyway a matter of figuring out the awkward corner cases, figuring out what people want in those cases, and covering with tests.

I realize though that this doesn't apply at all if people are manually writing augmentation libraries. That would be a pretty big foot gun. So maybe it's a non-starter ;) or maybe people should not manually write augmentation libraries...

[jakemac53]

It doesn't feel particularly different to other design/implementation issues macro authors face

It is different imo because it introduces unsoundness in the field promotion logic. If we were OK with that, we probably would have just shipped field promotion generally, as an unsound but useful feature. Fields are very rarely if ever overridden with unstable getters.

[jakemac53]

If a private getter is declared as abstract, and then a body is added in macro step 3, then that should also change another final variable of the same name from being promotable to not.

Hmm interesting example, but yes that sounds correct.

[lrhn]

you get undefined behaviour

Which means you get runtime errors.

Our native compilers won't be able to abide a type soundness violation, so if we stick with a promotion that is then invalidated by a macro step, then the compilers will need to test the type (or value identity, semantics of stableness aren't specified) of the promoted variable when it's read at a promoted type.
If something fails, you get a runtime error.
(So far, our users have generally preferred to not get runtime errors.)

I'd rather just give a compile-time error saying that your code promotes, the macro invalidates the promotion, so try to not promote. (At which point the author can rightfully say "no, YOU try not to promote". After all, they didn't ask for it, we just did it to them.)

Or rather, I'd rather re-analyze everything that we think needs it after a macro step. Whatever the program ends up with determines which errors you'll get.
If the final program makes the variable non-promotable, you get an error saying "variable not promoted" (or how we tell people they need promotion, but didn't get it).

If that changes the type of something, making the macro generated code be type invalid, that's the error you get. That's a bug in the macro. It assumed something that wasn't stable under macro modification.
(It's annoying if it's broken by another macro, but that's what happens when you mix code generators.)

We shouldn't try to be clever. That's ratholes all the way down, and can get in the way of future features too.

Give macros what code generation has had so far: The ability to inspect the program so far, to some level of detail, and try to derive from that what will happen when adding code to that. Then generate code, and see if it compiles.
Don't make any promises about what the final program will be. Give best-effort guesses based on current state. If something changes, then it changes. Only the final program really counts.

[jakemac53]

This already does cause problems for codegen authors today, here is one example of that. I do think it is worth trying to ensure we do not give a macro a wrong answer to a question they ask about the program. An incomplete answer is OK (and unavoidable), throwing is also OK (if unavoidable), but not a straight up wrong answer.

In general, it hasn't actually been particularly challenging up until this point to uphold those aspirations. The one special rule I can think of that augmentations (and thus macros) can't contribute to type inference. And that has been enough. Obviously, field promotion interacts with this such that even adding a body at all affects promotability, which affects type inference.

This does feel like an extremely niche scenario in general. You have to have a concrete class with a private final field, which is extended by a class that overrides the implicit getter from that field with an originally abstract getter, whose body is filled in by a macro. And on top of that, you then have to use that field in such a way that its promotability affects the type inference of a field through its initializer. I don't mean to dismiss this scenario entirely, but I do think it is sufficiently niche that simply throwing an error would be perfectly acceptable (at compile time).

[stereotype441]

I this a problem for plain getters too? If a private getter is declared as abstract, and then a body is added in macro step 3, then that should also change another final variable of the same name from being promotable to not. (That's an issue we've seen before - the inability to distinguish an abstract declaration from an "eventually given implementation" declaration until the implementation is added.)

Oh yeah, that would definitely be a problem too. Thanks for spotting that. I'll edit the original description to include this possibility. I guess in order to close this loophole we would have to prohibit augmenting an abstract getter with a concrete one, which seems like a little less of a nice use case than augmenting a final field with a concrete getter ☹️.

It doesn't feel particularly different to other design/implementation issues macro authors face

It is different imo because it introduces unsoundness in the field promotion logic. If we were OK with that, we probably would have just shipped field promotion generally, as an unsound but useful feature. Fields are very rarely if ever overridden with unstable getters.

Yeah, I agree that this is different because it's a soundness issue. Elsewhere in the language, we've been very diligent about avoiding unsoundness, and I think that's really important. It's one of the key differentiators between Dart and languages with more "gradual" type systems (e.g. TypeScript) that I proudly talk about when introducing Dart to developers who aren't familiar with it. Personally I would really prefer to weaken the macros and/or augmentations feature slightly to preserve soundness.

I don't have strong opinions about how we preserve soundness, though. If people feel like it's too limiting to fully prohibit final fields and abstract getters from being augmented by a concrete getter, I would definitely be open to something along the lines of what @lrhn suggested above. Concretely, perhaps after macro phase 3, we could re-analyze the type of any OmittedTypeAnnotation that was queried during phase 3. If the answer is different, we could issue a compile-time error pointing to the member whose type changed, saying "the inferred type of this member was changed from type T to type U by macro processing. To ensure a stable type, please give the member an explicit type of U." The analysis server could even have a quick fix to add the explicit type.

[jakemac53]

I guess in order to close this loophole we would have to prohibit augmenting an abstract getter with a concrete one, which seems like a little less of a nice use case than augmenting a final field with a concrete getter ☹️.

In particular, this is actually how a lot of macros are expected to work - in phase two they emit a getter with no body, and in phase 3 they fill in the body, since that is the safest/best time to do deep introspection.

[jakemac53]

Concretely, perhaps after macro phase 3, we could re-analyze the type of any OmittedTypeAnnotation that was queried during phase 3. If the answer is different, we could issue a compile-time error pointing to the member whose type changed, saying "the inferred type of this member was changed from type T to type U by macro processing. To ensure a stable type, please give the member an explicit type of U." The analysis server could even have a quick fix to add the explicit type.

Yes a solution such as this is in particular I think a good solution, at least from a usability perspective. We only error when this situation actually occurs, which should be very rare, and we can avoid limiting the feature.

If we go this route, I would be inclined to also allow augmenting implicit final field getters with getters, just to avoid the extra complexity of specifying that you can't. Even if I don't believe its something a macro should necessarily do 🤣 . There are imaginable situations where it could be done in a sound manner.

[jakemac53]

cc @johnniwinther @scheglov wdyt about the proposal to re-infer the type of any OmittedTypeAnnotation whose type was inferred, after macros are completely done, and emitting an error if that type changed?

[scheglov]

I don't like re-inferring, it does not fit well with the current analyzer implementation.
Maybe disable private field promotion when the are macro applications in the library?

[lrhn]

in phase two they emit a getter with no body, and in phase 3 they fill in the body, since that is the safest/best time to do deep introspection.

I suggested elsewhere that the macro could perhaps be forced to pre-declare whether it intends to add a body or not.
That is, in phase two, it emits either an abstract getter or a concrete getter with no body. It'll be an error to add a body to an abstract getter in phase 3, and an error to not add a body to a concrete getter.

That works for declarations entirely declared by macros, but not if the user writes the original abstract declaration and intends for the macro to fill in the implementation. That's where we currently use external in our patch files.

So, suggestion:

• A declaration can be concrete, abstract or external. A concrete declaration has an implementation. An abstract does not. And an external must have one (provided somehow) if it gets called, but doesn't have to if it doesn't.
• Phase two generation can choose to add an abstract or external declaration.
• An external declaration can be augmented (in phase 3) with an implementation. (If that implementation calls augmented, the original must have had a real external implementation, otherwise it doesn't matter.) That removes the external from the resulting declaration, which is now concrete.
• A declared abstract declaration cannot be given a body.
• A user wanting a macro to implement their own-declared getter can write @fillInTheBlanksMacro() external final int;.

could re-analyze the type of any OmittedTypeAnnotation that was queried during phase 3

Not necessarily enough. If someone adds, completely without querying anything, an implementation for int get _foo; on a macro-introduced class, it can still change the promotion behavior of an otherwise unrelated final int _foo; somewhere else.
That still makes the program not compile, even if nobody looked anywhere near the code that declares final int _foo; and the promoting reads. "Spooky demotion at a distance!".

It probably doesn't need "spreadsheet-level" dependency tracking to detect problems. Just knowing that a specific private instance variable is promoted as part of type inference of some member, should be enough to go back and recompute that if any non-promotable implementation of that name is added to the same library. (And if that makes any code downstream from the promotion invalid, or changes the type of any declaration, so bounded by the surrounding declaration, then it's an error.)

Still, I do worry that this is a problem that can affect future improvements to type inference too. Or make it hard to add any future promotions because it can then break macros.
The reason I so staunchly object to ideas the break getter/variable symmetry, or break interface abstraction by caring how something is implemented, is that any time we do that we make more changes be breaking. Usually changes we didn't want users to worry about. If macros cannot work with smarter promotion, then it makes smarter promotion a breaking change.

Macros look below the interface, at the implementation. As long as that only applies to the same library where the macro is applied, it's a localized issue.
We allow promoting private final instance variables inside a library, not because changing a variable to a getter isn't breaking, but because the breakage is restricted to the library currently being edited. It's immediately noticeable, and can be fixed by the person who broke it. The cost/benefit calculation comes out positive, because the cost and reach of breakage is limited.

Disabling private field promotion if macros are used can work, but has one issue: We only have on language semantics, which is unaware of macros. It just sees the result of applying macros to a source file, as a resulting library file with one or more augmentation files, then treats them as completely normal files with the one Dart semantics.
So we can't disable variable promotion when macros are used, because we don't have any language semantics without variable promotion to opt in to, and we don't have any concept of macros being used in the semantics. (And that's good, we don't want macros to have a different semantics that users also need to learn.)

(We could possibly choose to disable it based on library URI. If the URI scheme starts with dart-macro-generated+ (or where we ended with that), then variable promotion is disabled. But I'm not entirely sure which files will have such a URI. I'd say the original library file should be converted to a macro-generated file to add the augment "dart-macro-generated+..."; include predicate that wasn't in the original source file. But then any library which imports the macro-augmented library must also be changed from import "foo.dart"; to import "dart-macro-generated+package:my_package/src/foo.dart";, which means it is also not the original source file, and will itself need to have a different URI, and then disabling private variable promotion for all of these libraries doesn't sound nearly as great.)

[munificent]

All we should have to do here is disallow augmenting final field getters, which it is anyways pretty reasonable to disallow.

I think it's reasonable to want to apply a macro to a final field to, for example, log all accesses to the field.

Honestly, I think the original sin in all of this is field inference.

If a macro turned a final field into something non-promotable, that doesn't cause any problems for macros except that that same field could be used in some other field's initializer and thus to infer the type of the field. But if we didn't have field promotion in the first place, then it wouldn't matter whether a macro changed the promotability of any field. The compiler wouldn't use the promotability until it was doing inference inside function bodies which doesn't happen until after Phase 3 is complete.

If we didn't allow access to inferred field types during macro introspection at all—so you can access the annotated type if there is one, but that's in—would that fix this problem?

It would still be the case that a macro could change the promotability of a field. But that promotability wouldn't have any user-visible effect until after all macros have completed and function bodies are being inferred. It's already the case that macros can easily affect the way types are inferred in function bodies. (For example, by adding an overriding declaration to a subclass that narrows a return type.)

[jakemac53]

Moving this to the macros project - afaict this isn't an issue for normal augmentations, it is more about the macros feature and how it wants to introspect on incomplete programs.