Fix parsing interface as extern class

[as3boyan]

Fix parsing interface as extern class

Because of change I made earlier 840dcaa
plugin parsed interface as extern class, because interface was added there and extern modifier is optional there.

[as3boyan]

externOrPrivate* + (class | interface) = makes any interface an external class

[as3boyan]

externOrPrivate* means that it can be not even an extern and still can be parsed as extern

[as3boyan]

Or perhaps we can add a new type, extern interface

[as3boyan]

Also maybe changing priority

private topLevelDeclaration ::= classDeclaration
                              | externClassDeclaration
                              | abstractClassDeclaration
                              | interfaceDeclaration
                              | enumDeclaration
                              | typedefDeclaration

to

private topLevelDeclaration ::= classDeclaration
                              | interfaceDeclaration
                              | externClassDeclaration
                              | abstractClassDeclaration
                              | enumDeclaration
                              | typedefDeclaration

May solve this issue

[as3boyan]

Anyway I think externOrPrivate* may be changed to externOrPrivate+

[as3boyan]

Sorry, forgot to create a branch 03fbf32
What do you think about this?

[EBatTiVo]

Changing the priority to get the rules to work is fine. A comment about the ordering of a specific block would keep folks from re-introducing errors.

[as3boyan]

private topLevelDeclaration ::= classDeclaration
                              | interfaceDeclaration //set higher priority than extern interface
                              | externClassDeclaration
                              | abstractClassDeclaration
                              | enumDeclaration
                              | typedefDeclaration

Something like this?

[EBatTiVo]

Something like this?

If that compiles, then fine. I think, though, that you will have to add the comment on its own line immediately above the rule.

[EBatTiVo]

Don't you want to drop the 'interface' from the OR clause here? (e.g. change "('class' | 'interface')" to "'class'")
If you do, make sure you fix the pin=4 to pin=3 in the line below.

[as3boyan]

Do you mean allow extern modifier for interfaceDeclaration(as it used before this pull request), and remove interface from externalClassDeclaration? (additionally add check for AbstractHaxePsiClass.java/isExtern function for external interface?)
I not sure about external interface, but I think it should be close to regular interface, just with public fields
I read about those pins, and if I got it right, then I agree with you, we can use pin=3.

[EBatTiVo]

Maybe I'm getting a bit confused here...
The way it is now, "extern interface" and "extern class" are both externClassDeclarations, and "interface" without the extern is an interfaceDeclaration. I would think that all interfaces should be interfaceDeclarations, and classes should be classDeclarations.

But that aside, isn't the point of this change to make interfaces not be extern? I guess I have to go do some more reading.

[as3boyan]

Yeah, the main purpose of this pull was to prevent parsing regular interfaces as extern classes.

Don't you want to drop the 'interface' from the OR clause here?

I don't get what do you mean by this

Do you mean allow extern modifier for interfaceDeclaration(as it used before this pull request), and remove interface from externalClassDeclaration? (additionally add check for AbstractHaxePsiClass.java/isExtern function for external interface?)

I think this is alternative solution, also rules for external classes and external interfaces may be a lil bit different

I tested it in try.haxe.org,

extern class allow methods to have body(empty)
extern interface doesn't(so it might be closer to regular interface parsing rules)

Maybe it's complicated

[EBatTiVo]

Boyan, are you waiting for some commentary from me? I was waiting to see a change that makes all interfaces obey one set of rules and all classes another. (That's what I meant by "Don't you want to drop the 'interface' from the OR clause here?") I expect to see the rules like this:

externClassDeclaration ::= macroClassList? externAndMaybePrivate 'class' componentName genericParam? inheritList? '{' externClassDeclarationBody '}'
interfaceDeclaration ::= macroClassList? privateKeyWord? 'interface' componentName genericParam? inheritList? '{' interfaceBody '}'
externInterfaceDeclaration ::= macroClassList? externAndMaybePrivate? 'interface' componentName genericParam? inheritList? '{' interfaceBody '}'

[as3boyan]

Ok I made changes you mentioned.

[EBatTiVo]

If you merge master down into this branch and re-push, then perhaps we can get the build and test cases to pass. Then, you should also be able to auto-merge.

Once we get a clean build, this is approved for merge.

[as3boyan]

#255

[as3boyan]

Can I just merge two files? haxe.bnf and AbstractHaxePsiClass.java?

[EBatTiVo]

Yes and no. Yes, you can hand-merge the two files, but you still need to regenerate (without hitting the grammar-kit bug), update copyrights, make sure it all builds locally, and then push. That's essentially the tasks laid out in the "Use the command line" link under the "We can't automatically merge this pull request" message in the merge box.

[as3boyan]

I don't get why I some test cases in ClassDeclarationTest section look like this(Class http://i.imgur.com/3O06WHy.png.

Oh, I think I get it now, externOrPrivate ::= 'extern' | privateKeyWord is used only in enum and typedef declarations now, so it should be fine.

[as3boyan]

@EBatTiVo Please review, basically it's the same + 2 test cases + some updated test cases

[EBatTiVo]

Approved for pull.

Looking at this has got me thinking about how we could (and probably should) make the EXTERNORPRIVATE nodes become private so they don't show up in the AST. Something like this:

privateKeyword := 'private'
externKeyword := 'extern'
private externPrivate := externKeyword | privateKeyword
private privateExtern := privateKeyword | externKeyword
private externOrPrivate := privateKeyword | externKeyword | externPrivate | privateExtern

Then, we wouldn't have an ExternOrPrivate.getPrivateKeywordList API, just a HaxeClass.getPrivateKeyword(). And, the 'EXTERNORPRIVATE' nodes would no longer have to be traversed.

But, hey, if we get the compiler to do the parsing, we may not have to maintain the rules at all. (Or, we'll maintain a much simpler set that processes compiler output. We'll see how that goes.)

[as3boyan]

Yeah I think it's better to avoid ExternOrPrivate.getPrivateKeywordList API, I can add your suggestions/code to this PR if you want.

About compiler, if compiler could output tree with text ranges for each element, then maybe we could work with it if we could somehow override default parsing, there should be a way to do this, like they do in gen generated code.

[as3boyan]

Now I think about compiler that might be pretty good idea.

[as3boyan]

Maybe something like this

HaxeFile(0,800)
    CLASS(5, 50)
        VAR(8, 14)

Like we have in IDEA

But also to manipulate at ast level, like class.getRBrace(), we may need to know ast data(tokens and positions)

I wonder how it's implemented in Java or Python

[EBatTiVo]

Yes, I've been looking at just that. There is no choke-point in the compiler's parser (at least the version that TiVo currently uses) where we can just pick up and spit out the tokens as we see them. There is a place where we do all of the stream reading -- where we get the next token from the lexer -- but that gives us very little data about the actual token. We really need to place every token into the output stream as it is recognized and before it is acted upon. I'm re-reading that code though, to get a better feel for all of it.

[EBatTiVo]

I can add your suggestions/code to this PR if you want

It's a bit of work, since we'd have to update all of the tests that fail, too; the ones with golden masters. So, no, not to this PR. We should pick it up as a new issue at some point.

[as3boyan]

Maybe we could use current Lexer with it just fine

[EBatTiVo]

Maybe, but I don't see the point. They (the java-based plugin lexer and ocaml-based compiler lexer) are different enough to make that painful, and we still need the parser to work with it. It'll be easier to leave it all in one language.

I've also looked at the AST in the compiler's dump (-D dump) and it doesn't have anywhere near the amount and type of information we need. It's great for debugging the compiler and creating new back ends, but it's not useful to the IDE.

[as3boyan]

Another problem with it, that if file doesn't get compiled then we don't get any info, so it's complicated.

[EBatTiVo]

A bit. Compilers usually don't stop at the first error, though. I don't know if the Haxe compiler does or not. But we still should be able to get tokens even if the compiler can't recover enough to generate code. But, like you said, "it's complicated."

[as3boyan]

#258