dart-lang / language

Design of the Dart language
Other
2.61k stars 200 forks source link

Macros file access #3952

Open davidmorgan opened 3 days ago

davidmorgan commented 3 days ago

The spec currently includes file access

https://github.com/dart-lang/language/blob/main/working/macros/feature-specification.md#resources

it either needs splitting out of the feature, or implementing :)

lrhn commented 2 days ago

This sounds very much like "assets". Could something be shared? Or is it just an accident of both wanting to access bytes or strings?

I can see a macro having its own binary/string assets available. Then it doesn't need to read from files, they assets are created when the macro is built. If the applying package needs to provide data at compile-time, then it probably cannot use assets, and it needs to provide some reference to its own files. Probably only those inside lib/, since that's all someone else compiling your code has access to.

The API could be made a little more consistent. (I'd personally drop either the async or the sync version. If there is a sync version, I'll use it every time. I don't think a macro is going to be massively parallel, so loading something, more slowly, in the background isn't going to improve performance. I can be wrong. If so, have only the async version.)

/// A read-only resource API for use in macro implementation code.
class Resource {
  /// Either a `package:` URI, or an absolute URI which is under the root of
  /// one or more packages in the current package config.
  final Uri uri;

Don't allow absolute URIs under other packages' roots. The only files of another package that is guaranteed to be available when compiling are those under lib/ (under the package root), and those should only be accessed using package: URIs. That's the only stable access.

For files inside the current package (which is the package that applies the macro), it could be possible to access outside of lib, at least from macro applications that are themselves outside of lib/, but it should probably still be relative to an implicit root, not an absolute URI.

Generally, don't allow absolute references.

  /// Creates a resource reference.
  ///
  /// The [uri] must be valid for this compilation, which means that it exists
  /// under the root URI of one or more packages in the package config file.

package root URI.

  ///
  /// Throws an [InvalidResourceException] if [uri] is not valid.
  Resource(this.uri);

  /// Whether or not a resource actually exists at [uri].
  bool get exists;

As both sync and async?

  /// Read this resource as a stream of bytes.
  Stream<Uint8List> openRead([int? start, int? end]);

openRead([int start = 0, int? end])

  /// Asynchronously reads this resource as bytes.
  Future<Uint8List> readAsBytes();

Allow [int start = 0, int? end] here too, and for the sync version. Maybe also for the strings. Maybe especially for those. I could see myself putting all string resources into a single file, with an index at the start, or hard-coded offsets, and then read a string when I need it.

  /// Synchronously reads this resource as bytes.
  Uint8List readAsBytesSync();

  /// Asynchronously reads this resource as text using [encoding].
  Future<String> readAsString({Encoding encoding = utf8});

  /// Synchronously reads this resource as text using [encoding].
  String readAsStringSync({Encoding encoding = utf8});

  /// Asynchronously reads the resource as lines of text using [encoding].
  Future<List<String>> readAsLines({Encoding encoding = utf8});

  /// Synchronously reads the resource as lines of text using [encoding].
  List<String> readAsLinesSync({Encoding encoding = utf8});
}
jakemac53 commented 2 days ago

This sounds very much like "assets". Could something be shared?

"Assets" (assuming you mean flutter assets) are afaik a runtime only concept, things to be bundled in the final APK of your app and accessed at runtime. "Resources" are a compile time only concept for macros. They generally should not be available in the final application. So, I don't think they overlap.

Or is it just an accident of both wanting to access bytes or strings?

This is just a property of anything wanting to read files?

I can see a macro having its own binary/string assets available. Then it doesn't need to read from files, they assets are created when the macro is built. If the applying package needs to provide data at compile-time, then it probably cannot use assets, and it needs to provide some reference to its own files. Probably only those inside lib/, since that's all someone else compiling your code has access to.

The primary intended use case is people supplying files as input to macros (for example, a template or schema). We need to provide access to things outside of lib/ at least in the case that the macro is applied to a library not under lib/.

(I'd personally drop either the async or the sync version. If there is a sync version, I'll use it every time. I don't think a macro is going to be massively parallel, so loading something, more slowly, in the background isn't going to improve performance. I can be wrong. If so, have only the async version.)

The sync API might not be implementable in all situations, so I would probably go for the async one (this is what build_runner does).

Don't allow absolute URIs under other packages' roots. The only files of another package that is guaranteed to be available when compiling are those under lib/ (under the package root), and those should only be accessed using package: URIs. That's the only stable access.

For files inside the current package (which is the package that applies the macro), it could be possible to access outside of lib, at least from macro applications that are themselves outside of lib/, but it should probably still be relative to an implicit root, not an absolute URI.

We could possibly say something like "a macro applied under lib can only read package: URIs". I am not sure what that would look like to enforce.

But yes generally the intention here is to allow macro applications outside of lib to access files outside of lib.

Note that for assets, flutter does allow access to them outside of lib, even for other packages. One could definitely argue that is a bad thing though.

In general, these absolute URIs can only be reliably constructed from relative URIs anyways. They are just converted to an absolute URI under the covers. Attempting to do anything else will fail for a published package. And I don't particularly care if a user hard codes a weird absolute path for their own local development.

We could say that the actual resource API only accepts relative paths or package scheme URIs?

Stream openRead([int? start, int? end]);

What if this was the only API we exposed directly, and the rest was all extension methods? Would that be problematic?

lrhn commented 2 days ago

"Assets" (assuming you mean flutter assets)

I mean Dart data assets. They are also runtime only, but a macro that is running could use its own assets, or assets of its dependencies.

jakemac53 commented 2 days ago

They are also runtime only, but a macro that is running could use its own assets, or assets of its dependencies.

Ah, so still seems like a different feature then, although maybe we could share some APIs, I can take a look.

rrousselGit commented 2 days ago

One use-case to keep in mind is: Project-wide configuration files, such as build_runner's build.yaml It is placed next to the pubspec.yaml, and needs to be read by macro applications inside /lib.

I'd expect macros to be able to read anything that's at the pubspec level or lower.

davidmorgan commented 2 days ago

FWIW, this looks like something we could safely move to the "Later" milestone and out of the first feature spec :)

rrousselGit commented 2 days ago

As in, not have this in the initial stable release?

I do think it's quite important to be able to define a global configuration file. Lots of generators have global configurations. Removing such a file would make migrating to macros a bit painful.

davidmorgan commented 1 day ago

Yes. As mentioned elsewhere, we are looking for ways to reduce the feature scope so we can launch sooner, and this looks like an easy one: the functionality is orthogonal and straightforward, and the feature works fine for many use cases without it.