Skip to content
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

Typed metadata (again!) #111

Open
wants to merge 12 commits into
base: master
Choose a base branch
from

Conversation

SomeRanDev
Copy link
Contributor

@SomeRanDev SomeRanDev commented May 17, 2023

Another typed metadata proposal.

Similar to previous proposal with relevant discussion here. Relevant wiki content here. Based off of @nadako's comment which seems to be the most popular idea.

// MyMeta.hx
@:metadata function author(name: String): haxe.macro.Expr.TypeDefinition;

// Sleigh.hx
@:MyMeta.author("Santa")
class Sleigh {
    // ...
}

Rendered proposal

@kLabz
Copy link

kLabz commented May 17, 2023

Why requiring the use of Context.getDecoratorSubject() instead of getting it as argument?

@skial skial mentioned this pull request May 17, 2023
1 task
@:metadata
@metadataCompileOnly
@metadataRequire(@:metadata _)
function metadataCompileOnly(): haxe.macro.Expr.Function;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An alternative would be to use @metadata function ... to define a runtime metadata (@foo), and @:metadata function ... for a compile-time one (@:foo). Requiring to add @metadataCompileOnly to all compile-time metadata sounds very annoying (also, we have namespaces).

Drawback is that it's easy to make a mistake between @:metadata function and @metadata function but that should be caught very easily at use site.

Copy link

@kLabz kLabz May 17, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also, we have namespaces

Btw, this seems like a limitation of this proposal to not handle metadata namespaces, which is unfortunate. (edit: I guess it's in concurrence with fully qualified path there..)

Copy link
Contributor Author

@SomeRanDev SomeRanDev May 17, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(edit: I guess it's in concurrence with fully qualified path there..)

Oh yeah, sorry if I didn't make it as clear in the proposal but you import like normal functions, otherwise you use full path. Like how you would with a normal function call. I kind of touch on it a little here, demonstrating how using module-level vs static function changes how you'd write it with basic module import.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Drawback is that it's easy to make a mistake between @:metadata function and @metadata function but that should be caught very easily at use site.

Yeah, this was certainly a tough thing to find an answer to. Maybe typed metadata should just support being used both ways. But if there's a colon, it doesn't generate rtti? Hmmm...

@SomeRanDev
Copy link
Contributor Author

Why requiring the use of Context.getDecoratorSubject() instead of getting it as argument?

This was actually my original idea. Instead of @:metadata, you could have metadata functions work as static extensions, so any static function with DecoratorSubject as the first parameter is a metadata function. Then you could use extern to generate metadata functions without bodies.

The only reason I didn't commit to that in this proposal is I feel like body-less metadata functions are going to be used waaaay more frequently, so you end up writing much more to accommodate the less common use case? Plus nadako's comment seemed kinda popular. And finally, Context.get____ feels like a standard way compile-time functions get information honestly (getBuildFields, getLocalClass, getLocalTVars, etc.).

But I'm glad you brought it up cause I thought that might be a better way too, and I'd be interested to hear what others think.

@kLabz
Copy link

kLabz commented May 17, 2023

I don't know, really. All those switch + case _: throw "Impossible"; etc seem pretty silly.
We could easily have a different signature for body-less metadata for those not to need to add a useless argument.

Speaking of body-less metadata, they seem pretty much useless as-is (unless we enforce typed metadata everywhere which I really doubt we'll do by default) as they don't even really ease reading their data from outside. For example I would assume we could somehow read something like e.meta.author.name from @:author("Santa") blargh expression with @:metadata function author(name:String):Expr.

@SomeRanDev
Copy link
Contributor Author

That fair. 🤔

something like e.meta.author.name

I like this a lot. I'll try and incorporate something like this into the proposal in a bit. meta already field on TypeDefinition/Field/etc, so how about something like metaArgs? (td.metaArgs.author.name)

Would need a way to handle having the same meta used multiple times... so maybe make it an Array in that case? td.metaArgs.author[0].name

@kLabz
Copy link

kLabz commented May 17, 2023

I may be wrong but I remember reading something about the metadata function having some way to allow being used multiple times or not? That could potentially be used to allow either .author.name or .related[0].link. Typing there could be a bit annoying though; if so then I guess we could go with mandatory array access.

meta already field on TypeDefinition/Field/etc, so how about something like metaArgs? (td.metaArgs.author.name)

We could have x.meta (current one) with raw data and x.typedMeta with typed metadata entries (allowing things like td.typedMeta.author.name)

@Aurel300
Copy link
Member

Merging typed metadata into the existing syntax seems unlikely to work out. In particular:

For the time being, this proposal suggests throwing an error unless -D allow-untyped-meta is defined.

Does this mean that virtually all metadata everywhere would be rejected unless the flag is set, especially metadata coming from third-party libraries (which might not update at the same time as the compiler)? Not good…

Or is this saying, if the compiler fails to type a metadata, when a function for it exists and is resolved, it will complain? Also bad: a typo in the metadata name (or a forgotten import, etc) will simply be accepted silently.

Also: in your proposal you only use the runtime metadata syntax @foo and never @:foo—would there be a way to discern these? Is there any value in keeping metadata like this in RTTI?

IMO we need separate syntax for this. I think @. has been proposed before.

@kLabz
Copy link

kLabz commented May 18, 2023

IMO we need separate syntax for this. I think @. has been proposed before.

Also Simon proposed only allowing classes as metadata providers and disallowing "untyped" metadata starting with an uppercase letter. That handles the separate syntax issue.

* Provide new suggestion for handling untyped metadata errors.

* Add `Array` and `Dynamic` to allowed arguments

* Remove `@:metadataCompileOnly`, `@:metadataRequire`, `@:metadataRequire`, `@:metadataPlatforms`

* Add options argument to `@:metadata`:
```haxe
@:metadata({ allowMultiple: Bool, compileTime: Bool, platforms: [] })
```

* Replace `MetadataEntryTools` with `typedMeta: StringMap<Dynamic>` field on certain structures.

* Added `Context.typeMetadata` function
@SomeRanDev
Copy link
Contributor Author

Updated proposal:

  • Reworked untyped metadata section. Untyped metadata will only throw error if the module uses any typed metadata.
  • Removed MetadataEntryTools, use typedMeta: StringMap<Dynamic> on AST structures to extract arguments.
  • Added Array<T> and Dynamic to allowed argument types
  • Removed extra metadata for configuring @:metadata, instead uses optional "options" argument:
@:metadata({ allowMultiple: Bool }) function author(name: String): Any;
  • Typed metadata that use @: must use @:metadata({ compileTime: Bool }). Tried to incorporate more examples of this.


### Reading Arguments

There needs to be a mechanism for reading metadata arguments. To provide this, a new field `typedMeta: StringMap<Dynamic>` should be added to:
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

StringMap<Dynamic> is very far from my idea of typed metadata :/
Though, well, it's not that easy to express with our type system but shouldn't be impossible.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahh, sorry. Could you give a more clear idea? I thought you just meant a Dynamic originally. How would your system handle the scenario where there are two metadata with the same name? Like in the example: Module1.myMeta + Module2.myMeta?

proposals/0000-typed-metadata.md Outdated Show resolved Hide resolved
proposals/0000-typed-metadata.md Show resolved Hide resolved
### Untyped Metadata

If the Haxe compiler encounters a metadata entry it cannot type, its behavior is currently an [Unresolved Question](0000-typed-metadata.md#unresolved-question).

For the time being, this proposal suggests throwing an error _unless_ `-D allow-untyped-meta` is defined.
For the time being, this proposal suggests printing an error for each metadata entry that could not be typed (no metadata function could be found for its name/path) ONLY IF within a module that meets the following conditions:
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still don't think it's ok as default behavior.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeahhh, idk. This is mostly meant to be unresolved until the main question of whether @. or @CapitalLetter or some sort of new indicator should be used. Idk, like with a lot of things not married to it, just trying to throw out solutions. 🤔

@SomeRanDev
Copy link
Contributor Author

SomeRanDev commented May 18, 2023

For the record, I favor the @. syntax. The reason it's not being proposed is because I'm trying to base this off the most 👍'ed comment on the previous proposal. Maybe I misread the general consensus cause I thought that having a new syntax seemed kind of unpopular (hence why it didn't pass?), I would love to just write that and have it solve all the typed vs untyped compatibility issues.

@wartman
Copy link

wartman commented May 19, 2023

What if type checking metadata worked sorta like null safety does? That might give the most flexibility for backwards compatibility without needing to introduce all kinds of new syntax. Make it opt-in per package or even per class, basically.

package my.package;

@:metadata({ rtti: true }) function foo(arg:String):Any;

// Could be something like this (at a class-level) or `--macro metadataTypeChecking('my.package', Strict)` (as an
// initialization macro). `Strict` could throw errors, `Warn` could just warn, `Off` could turn it off completely?
@:metadata.typeChecking(Strict)
class Foo {
  @foo('ok') final foo:String; // Fine
  @bar final bar:String; // Throws an error

  // etc.
}

Alternatively this could be opt-out, allowing you to turn off type-checking for various packages (but even then maybe only emit a warning by default?).

This probably won't work too well with the more ambitious parts of this proposal, but if all we really want is some completion and error checking, maybe it's enough? Heck, even if we go with a new syntax like @. having some way to check all metadata seems like it would be nice.

Edit:
I'll also say, just as personal preference, that I think it would be neat if we could get rid of all special syntax (or at least make it optional) for metadata if we're in a typedMetadata(Strict | Warn) context. The compiler will already know if the metadata is runtime or not since we'll have { rtti: true|false } in the metadata definition, meaning that adding the extra : for compiler metadata would be redundant. This is perhaps mostly an aesthetics thing, but it seems like it would be a reasonable addition and would avoid some unnecessary ceremony.

@Simn
Copy link
Member

Simn commented Nov 21, 2023

Some remarks:

Metadata targets as return types

This is a bit too clever. The main problem I see is that we cannot express the targets of something like @:final, which currently has "targets": ["TClass", "TClassField"]. It also looks somewhat alienating to deal with types from haxe.macro.Expr here. It would be better to just add the targets as an argument like a normal person would.

@:metadata bootstrap

I guess this is mostly a note to myself for when I start implementing this, but @:metadata function metadata looks like a fun time. This has some implications for how we handle typer passes, because we can't really load the metadata function while typing the metadata function.

Decorators

Ok listen... I can see why you would want that, but it's a bit too much for a typed metadata proposal. At least at the first step. Metadata is (supposed to be) something very declarative, and executing macros for each of their occurrences is not. I would forego this entire part for now.

Typed metadata activated like null-safety

I find @wartman's idea quite interesting. This might be a good way to approach the issue indeed, especially because it allows "upgrading" untyped metadata to typed metadata. I'm not sure yet how this should be brought into scope (I think it should be at file/directory-level, probably utilizing import.hx), but my initial reaction that this is definitely the way to go.

One problem is that for some metadata, typing is required. I've been dealing with how our @:strict currently works recently, and it does some transformation by typing its arguments and then adding a modified @:meta. This means that some kind of metadata will always require typing, which should be considered.

This would mean that we always try to find a metadata definition, and whether or not we error out if there isn't one depends on the mode we're in. I'm not sure if we really need a mode where we find a definition, but accept that it doesn't type properly. I'd estimate that this would mostly come from metadata naming conflicts.

@skial skial mentioned this pull request Nov 28, 2023
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants