How do we handle access to global mutable state in macros?

[munificent]

We've discussed this in meetings and the proposal lays out several options, but I didn't see a tracking bug. Filing one now. Options from the proposal:

1. Don't allow macro code to mutate global state at all. This is probably
   overly-restrictive, and may be hard to enforce. There are legitimate use
   cases for this like like using package:logging.

2. Run each macro application in a completely new isolate. Each application
   has its own independent global mutable state. This is permissive in macros
   while keeping them isolated, but may be slow.

3. Reset all static state between macro application executions. If this is
   feasible to implement and fast enough, it could work.

4. Document that mutating global state is a bad practice, but don't block it.
   Give no guarantees around static state persistence between macro
   applications.

   In practice, most macros won't access any global state, so this is harmless.
   But if macros do exploit this (deliberately or inadvertently) then it could
   force implementations to be stuck with a specific execution order in order
   to not break existing code. This is the easiest and fastest solution, but
   the least safe.

[jakemac53]

My thoughts:

Option 1

It feels both too restrictive and not worth the effort of trying to enforce

Option 2

I can't imagine this is fast enough - it seems that our time budget per macro application is <1ms. Even that budget could represent a significant total increase in compile time if macros were used at a large scale.

Option 3

I think the cost/benefit analysis just doesn't work out.

Option 4

This is my choice. Ya, its not perfect, but it will result in the fastest compilation and there are legitimate use cases that don't violate the rules. The build_runner package gets away with this approach, as does bazel (local workers in bazel are long running, have persistent state, and bazel just trusts them to do no wrong).

[munificent]

I see the appeal of 4, but I'm very hesitant to commit to it.

JavaScript never specified object property iteration order and left it up to implementations. Chrome chose an iteration order different from insertion order and it became a never-ending series of bug reports because users expected it to work the same across all browsers. Eventually, ECMAScript conceded defeat and specified an iteration order in ES 6. The Go team didn't want users to inadvertently rely on map iteration order so they actually randomize it every time to prevent users from relying on it.

This is why the default map type in Dart is LinkedHashMap (specified iteration order) and not HashMap (unspecified).

I worry very much that if the macro expansion order is ever visible, we will be forced to maintain that order in perpetuity and that could make it harder to optimize the compiler. Maybe the right answer is to do what Go does and randomize the order.

[jakemac53]

I would be fine with randomizing macro ordering or something in order to try and prevent users from doing weird things, that should be pretty cheap to do.

[jakemac53]

We could also try and do something where we periodically (maybe randomly) do reset the state (hot restart the macro isolate or something randomly).

[jakemac53]

I am going to close this issue - the spec currently has this:

A well-behaved macro should not:

*   Reach out to the network or read arbitrary files on the user's machine.
    (Some files can be access by going through the Resource API because the
    user knows those files are permitted.)

*   Consume CPU resources or continue executing code after the macro's work is
    done.

*   Produce different results when run multiple times on the same code. A macro
    should be idempotent, and should always generate the same declarations from
    the same inputs.

*   Use mutable global state to pass objects or information derived from one
    phase to a macro (even itself) running in a later phase.

The macro system is *not* designed to provide hard security guarantees of the
above properties. Users should consider the macros that they apply to be trusted
code.

The restrictions in the following sections encourage the above properties as
much as possible.

Which is Option 4. I think that is fine.

[rrousselGit]

@jakemac53 Just to be clear, that doesn't prevent macros from defining global state if there's no phase issue, right?

For example what if two macros depend on a package which has defines a global variable. Assuming both macros are correctly in the same phase, is it legal and is the state shared between macros?

The point being that we'd want to enable to cache some logic across macros when possible.

[jakemac53]

Correct, package:logging for instance uses factory constructors and re-uses loggers based on the name.

This type of behavior is allowed, but the behavior is ultimately undefined and a macro should not be written in a way that requires the caching.