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

No error when typedef is missing semicolon #10822

Open
tobil4sk opened this issue Oct 15, 2022 · 4 comments
Open

No error when typedef is missing semicolon #10822

tobil4sk opened this issue Oct 15, 2022 · 4 comments
Assignees
Milestone

Comments

@tobil4sk
Copy link
Member

This is permitted in Haxe:

typedef A = String

I think it is a bug to do with allowing:

typedef A = { a: String }

The code seems to be found here: https://github.com/HaxeFoundation/haxe/blob/development/src/syntax/grammar.mly#L284-L286

We should instead do something like we do here:
https://github.com/HaxeFoundation/haxe/blob/development/src/syntax/grammar.mly#L100

This is inconsistent with the rest of the language, and causes issues with the vshaxe parser:
vshaxe/vshaxe#24
vshaxe/haxe-TmLanguage#54

@Simn
Copy link
Member

Simn commented Nov 7, 2022

I checked the blame because I thought this was something broken more recently, but it's from 2007: e7aae26

I agree though, omitting the semicolon here is wrong because it's supposed to be optional only after }. My concern is that this might break some ancient libraries that nobody really maintains anymore, but I suppose there's only one way to find out.

@Simn Simn self-assigned this Nov 7, 2022
@Simn
Copy link
Member

Simn commented Nov 7, 2022

Well, turns out there are several places in our standard library and tests already, which doesn't surprise me.

IMO it's better to just accept this and support it in vshaxe. Fixing this might technically be "correct", but I don't really see how it makes anyone's Haxe life any easier.

Edit: I've pushed the branch so that we can gauge the fallout.

Simn added a commit that referenced this issue Nov 7, 2022
@tobil4sk
Copy link
Member Author

tobil4sk commented Nov 7, 2022

I guess at least it would be a pretty unambiguous error message. I just think that if we're going to have optional semicolons in some places then the rules should be consistent.

Looks like it does break tink though, so maybe it is a bit problematic, however, it is a really simple fix. Maybe it could be considered for Haxe 5? If any of the other parsing issues end up being resolved it would be nice to have a general parser cleanup for that release, because right now inconsistency like this makes writing external parsers annoying, especially with tools that are more strict about grammar correctness. I guess this case isn't too bad though, because as far as I can tell it shouldn't cause ambiguity.

@Simn
Copy link
Member

Simn commented Nov 9, 2022

Yeah I don't think we should change this in Haxe 4.

@Simn Simn added this to the Release 5.0 milestone Nov 22, 2022
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

No branches or pull requests

2 participants