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

@:native for structure fields #32

Closed
wants to merge 12 commits into from

Conversation

nadako
Copy link
Member

@nadako nadako commented Oct 4, 2017

Rendered version


typedef JsonSchema = {
  @:native("enum") var enum_:Array<JsonValue>;
}

schema.enum_ = [1,2,3]; // generates schema.enum = [1,2,3]
print(schema.enum_); // generates print(schema.enum)

See HaxeFoundation/haxe#5105 and HaxeFoundation/haxe#2185

@nadako
Copy link
Member Author

nadako commented Oct 4, 2017

I can work on implementing this, but I need some review and approval of the idea by @ncannasse and/or @Simn first.

@ncannasse
Copy link
Member

I think an easier version would be to allow { "enum" : 5 } and have it typed as { "enum" : Int } (both would be allowed in terms of Haxe syntax).

In that case it might still be escaped as _enum or $enum or something else if enum is a keyword in the generated platform.

@nadako
Copy link
Member Author

nadako commented Oct 5, 2017

I think an easier version would be to allow { "enum" : 5 } and have it typed as { "enum" : Int } (both would be allowed in terms of Haxe syntax).

That would also require some field access syntax like schema."enum", but yes that would also do the job. Althought I thought you were against exactly this in HaxeFoundation/haxe#2185 and asked for the @:native proposal.

What should we do?

@ncannasse
Copy link
Member

ncannasse commented Oct 5, 2017 via email

@nadako
Copy link
Member Author

nadako commented Oct 5, 2017

we don't want to allow either value."enum" or value["enum"]

While I can see why we don't want the general value["enum"] syntax (since it looks like a map lookup), I'm not sure why you're against value."enum" - it's clearly a field access and doesn't suggest any map-like behaviour. I consider this kind of identifier escaping.

Btw, I think C# has value.@enum syntax for this, which could be an option too (tho maybe can be a bit confusing wrt our metadata syntax).

@nadako
Copy link
Member Author

nadako commented Oct 5, 2017

Anyway, I'm waiting for directions on what to do next: proceed with the proposed @:native stuff or implement the quoted fields syntax.

@clemos
Copy link

clemos commented Oct 5, 2017

While I think it would be nice to be able to use value.enum directly (I have no idea but maybe it would be possible to differentiate field names and keywords ? In JS, using such keywords as field names is perfectly valid), I think it should be done in another feature request.

@ncannasse
Copy link
Member

@Simn what do you think about us allowing <expr>.<keyword> ?

@Simn
Copy link
Member

Simn commented Oct 11, 2017

Well, let's talk about consistency here:

  • For fields on types, we have to use @:native("keyword") metadata
  • For structure values, we can use quoted fields: { "keyword": value }
  • For field access, we would use expr.keyword

That doesn't strike me as good design. Allowing expr."keyword" would at least be consistent with the structure values, and also at least similar to the type field version because "at least they both use string literals".

@kevinresol
Copy link

kevinresol commented Oct 12, 2017

I am bringing this up again: #22.

I suppose in some other languages there are concepts of contextual keywords (e.g. await in C#), that some identifiers are only considered as keywords in particular contexts.

IMO, we could implement that step by step. As starter, field.keyword should be allowed as there is no ambiguity. (though how to define the field is another topic)

@clemos
Copy link

clemos commented Oct 12, 2017

@Simn Ultimately, I think it would make sense to allow { keyword : value } too (once again, like JS does).
Fields on types certainly bring more issues, but I think we could consider allowing them too (could only be accessed via this.keyword or Class.keyword though)
Anyway, I agree with @kevinresol : #22 is probably worth reopening.

@nadako
Copy link
Member Author

nadako commented Oct 18, 2017

can we go with @:native solution that works for now and then you guys discuss new syntax?:)

@ncannasse
Copy link
Member

I fail to see how that would even work, for instance with this:

typedef Args = { var x : Int; @:native("new") var y : Int; };

class Test {
    static function foo() return { x : 0, y : 0 };
    static function main() {
         var v : Args = foo();
    }
}

If it's just to allow assigning literals I think it's not good enough.

@kevinresol
Copy link

kevinresol commented Oct 19, 2017

The fun fact is that on js you can use keywords as object field:

var o = {new:1}; console.log(o.new); this runs fine in js

@clemos
Copy link

clemos commented Oct 19, 2017

@ncannasse well @:native already has this problem https://try.haxe.org/#36B22 so this wouldn't really be a con for this feature :S
Obviously, I think everybody agrees allowing keywords as field names would be cleaner...

@nadako
Copy link
Member Author

nadako commented Oct 19, 2017

Seriously, people? I covered this exact case in the proposal...

@kevinresol
Copy link

kevinresol commented Oct 19, 2017

Seriously, people? I covered this exact case in the proposal...

Sorry I misunderstood Nicolas's comment. Yeah, the example case makes sense.

@kevinresol
Copy link

I think the keyword stuff is a bit off-topic here, sorry for bring that out at first place. (but I still hope it could be re-considered)
Personally I think OP is a good solution with least effort in current situation.

@ncannasse
Copy link
Member

Ok, I forgot that you have dealt with it in the proposal, sorry.

Reading again the whole thing, while I'm still not 100% satisfied with the solution, I don't have either a better proposal to make at this point, so feel free to move forward with the change.

@kLabz
Copy link

kLabz commented May 1, 2018

Any news on this?
Would be much appreciated when dealing with externs for react libs :)

@hamaluik
Copy link

I also need this for dealing with externs for various projects, most lately: Blender externs.

@nadako
Copy link
Member Author

nadako commented Jun 11, 2018

I have a branch somewhere... Will try to find, revive it and get into Haxe 4

@Simn
Copy link
Member

Simn commented Jun 11, 2018

It bothers me that this affects unification... It shouldn't be necessary to have typing jump through hoops because the syntax is insufficient to express what you want to express. It's not even strictly related: The shortcoming here is in the field-access syntax, and our solution is to meddle with the struct declaration and its unification behavior.

I don't know if we're still voting, but this one would get a "meh" from me. I acknowledge the problem, but I don't think this is the best solution.

@nadako
Copy link
Member Author

nadako commented Jun 11, 2018

Well, but the problem is severe and the fix like this, even temporary would be appreciated, also I don't think the change in the code base will be that intrusive. Anyway, suggestions are always welcome :)

@Simn
Copy link
Member

Simn commented Jun 12, 2018

I still think field."name" is the obvious solution here. It solves exactly what we want to solve and whatever problems it brings at generation-level would also be brought by @:native.

@hamaluik
Copy link

I don't know anything about the compiler or unification, but I can add from a user's perspective the original proposal matches exactly what I expect to happen based on how @:native works in other extern places (and I was a bit surprised at first when I naively tried to apply @:native on a field name and it didn't work like it does elsewhere). I get that "working the same as elsewhere" might not be strictly correct from a compiler / language standpoint, but I think it meshes with the intention of the @:native metadata (to transform Haxe types into native types, which would be expanded into: transforming Haxe types and fields into native types and fields).

I think the field."name" syntax is a bit confusing—it suggests to me that I could do something like field.'name${5*25}', or even var s:String = "abc"; field.s = true; (what would it export as? field.s or field.abc? Or throw an error?)

@markknol
Copy link
Member

markknol commented Aug 1, 2018

I'm in favor of allowing @:native on typedefs. It would help on the my hxobfuscator project, which shortens/obfuscates field names by decorating fields with @:native.

markknol added a commit to markknol/hxobfuscator that referenced this pull request Aug 1, 2018
…in a context

* still there are weird errors but code is less complex
* noticed when using typedefs, everything is broken. Give your vote HaxeFoundation/haxe-evolution#32
@kLabz
Copy link

kLabz commented Sep 2, 2018

It seems like we will need some sort of solution to this problem in the near future for haxe js (well, react): React will most probably drop className in favor of class in the next major version (discussion here) (+ for instead of htmlFor).

I'm not even sure @:native would be enough for this use case, since when doing jsx('<div class="myclass">hello world</div>') there isn't any typedef for the props atm. Well, this one needs a macro anyway so it could be handled somehow.

@Simn
Copy link
Member

Simn commented Sep 3, 2018

or even var s:String = "abc"; field.s = true; (what would it export as? field.s or field.abc? Or throw an error?)

How would field.s ever resolve to field.abc or throw an error? It's the normal field syntax and totally unaffected by anything.

Anyway, it feels like I'm on a different planet than everybody else here. So if you want to go ahead with @:native I'm not gonna stop you.

@kLabz
Copy link

kLabz commented Sep 3, 2018

expr."keyword", while not being pretty (imo), may be the cleanest (and more robust) solution indeed.

It also solves the field."not-valid" case @clemos and @nadako mentioned, which may come up in the future (for js target at least).

I think the field."name" syntax is a bit confusing—it suggests to me that I could do something like field.'name${5*25}'

We'd just need an error stating that single quotes are not allowed for quoting field names. Or allow string interpolation here..?

As for @:native use on typedefs, I think we need a compilation warning (which can be disabled?) telling us that it only works on types (with a hint to the quoted alternative). I'm not sure it is clear enough atm that it won't work on typedefs, and it just fails silently (until a runtime error occurs).

@kevinresol
Copy link

kevinresol commented Sep 3, 2018

Or allow string interpolation here..?

You can't interpolate in field names in ObjectDecl already {'$x':1}, so I think it is ok.

And it is quite consistent as well. I mean field."keyword" and {"keyword": value}

@kLabz
Copy link

kLabz commented Sep 3, 2018

Yep 👍

What about typing this with typedefs? Would it be typedef A = { "keyword":String } / typedef A = { var "keyword":String; }?

@nadako
Copy link
Member Author

nadako commented Sep 3, 2018

I'm gonna close this proposal as it's clear that we don't have consensus. As I mentioned before, I think obj."field" is a nice(r) option as well, I just thought it was rejected before. Anyway, this will require another design proposal that someone has to make.

@nadako nadako closed this Sep 3, 2018
@skial skial mentioned this pull request Sep 3, 2018
1 task
@kLabz
Copy link

kLabz commented Sep 4, 2018

@ncannasse

we don't want to allow either value."enum" or value["enum"]

Do you have a strong opinion against value."enum"? If so, could you elaborate a little?

@farteryhr
Copy link

farteryhr commented Jan 15, 2019

Yep 👍

What about typing this with typedefs? Would it be typedef A = { "keyword":String } / typedef A = { var "keyword":String; }?

just thinking more on induction:
var "keyword": String; (ok in typedef. what about in a class? what about in a block?)
"keyword": String (ok in typedef, what about in an argument list?)

and more idea (mostly a general identifier quote):

  • the extravagant solution: backtick
  • $"keyword"
  • \keyword \"key-word"

oops

i think i found something good? any cons please?

  • \ is currently an invalid character outside string literals.
  • single extra char.
  • has some common meaning of "escaping". also there is similar buf different use case in js var \u4e00 = 1; which will be \"\u4e00" here though.
  • simple to parse, works everywhere, just lexical change, no contextual info needed.
  • even compatible with string interpolation '$\keyword, $\"key-word", ${\if+\else+\"if-else"}'.
  • in fact no conflict with @nadako's proposal. (lol \"off-topic")

examples:

typedef JsonSchema = {
  var \enum:Array<JsonValue>;
}

schema.\enum = [1,2,3];
print(schema.\enum);

var \var = 1;
const { \class, \for } = prop;

// maybe allow omitting \ on "xxx" when possible?
someHaxeDiv.style.\"background-color" = rgb(234,130,32); // just demo
{\"background-color": rgb(234, 130, 32)}

@nadako
Copy link
Member Author

nadako commented May 16, 2020

FWIW I implemented field."name" syntax here HaxeFoundation/haxe#9433. Can't say I'm a huge fan of this, but we need some solution.

@markknol
Copy link
Member

So this issue can only be tackled at the call side and not at the definition side?

@nadako
Copy link
Member Author

nadako commented May 18, 2020

So this issue can only be tackled at the call side and not at the definition side?

With @:native solution, it's just the definition. With field-quoting solution it's both definition and access.

@markknol
Copy link
Member

Ah that sounds great!

@sonygod
Copy link

sonygod commented Nov 2, 2020

I can't wait for this.

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.

None yet

10 participants