Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Fix all compilation issues in parser #16

Closed
nesteruk opened this Issue · 24 comments

4 participants

@nesteruk
Owner

Currently, the generated parser has >200 issues caused by wrong namespaces and missing types. SHOWSTOPPER

@nesteruk
Owner

@ollka if you suddenly get time, mind taking a look? no need to fix everything, just general direction is needed. you're most qualified to comment on this.

@ollka ollka was assigned
@nesteruk nesteruk referenced this issue
Merged

"Bang" keywords #31

@OkayX6

Hi @nesteruk,

I took some time to analyze & classify the errors encountered.
Wouldn't it be simpler to fix the fsharp.psi file?

Basically, most errors can be classified as is:
1. in src/Gen/Tree, most of the interfaces inherit $extend, which is obviously non-compilable code
2. In src/Gen/Impl, a lot of classes have a double interface implementation (ex: AlgebraicTypeRepresentation.cs)
3. they override methods that do not exists in a base class
4. they override Accept(...ElementVisitor visitor) + the generic version, and ElementVisitor doesn't appear to have been generated or created by the tool. Actually this might be caused by the error listed in bullet 5
5. most classes inherit unexisting classes such as:

  • ExpressionBase
  • InferenceErrorBase
  • InferenceTypeBase
  • OperatorExpressionBase
  • PatternBase
  • ReferenceBase
  • ReferenceExpressionBase
  • TypeExpressionBase
  • TypeVariableScopeBase

Maybe you can get better directions on how to fix this?

Thanks

@citizenmatt
Owner

Yep, that's exactly it. Unfortunately, the psi file format is sorely lacking in documentation, so it's very hard to know how the file format influences the generated parser code, and also the generated interfaces and stubs, and how the generated code relies on and interacts with code that's written by hand.

The PsiPlugin in the SDK provides syntax highlighting and error analysis on .psi files, and shows lots of errors in the fsharp.psi file. The psi.psi file in the PsiPlugin is a grammar that describes the format of .psi files, which is useful, but you still need to infer the psi file format by reading a .psi file!

I'm currently looking into this, trying to pull together enough notes to count as documentation. Hopefully, this should allow us to see what needs to change in order to get everything to compile correctly.

@nesteruk
Owner

@citizenmatt do you think we should be formally documenting this, perhaps in the ReSharper Plugin Dev Guide?

@citizenmatt
Owner

That's what I'm working towards. Well, both - get it working, and get enough notes along the way to be able to document it to a reasonable level.

@ollka
Owner
@nesteruk
Owner

@ollka thanks a lot for the comments! Indeed we may end up doing just that, i.e. wipe out the parser altogether and start anew. It's a pity that the PSI format didn't get documented anywhere, its specifics are now a kind of tribal knowledge :)

@OkayX6

Thanks all for your comments.

Just to clarify, I'd like to know which direction we're going to:

  • Rewrite PSI file to fix generation?
  • Or, write a parser from scratch by hand?
@citizenmatt
Owner

My vote is to use automatic generation - get the psi file working. I'd rather work from a file that contains a grammar, rather than have the grammar spread across the implementation of a lot of files, especially as those files don't actually describe the grammar, but are about building the PSI semantic tree.

@nesteruk
Owner
@OkayX6

Do you have a simple PSI example file to start with?

@ollka
Owner
@citizenmatt
Owner

The only example .psi file at the moment is in the SDK - check out the PsiPlugin example. It has a psi.psi file, that is used to generate a parser for .psi files.

@OkayX6

Hi,

I gave up looking at PsiPlugin.
Instead, I have started a new fsharp.psi from scratch called fsharp-new.psi.
It's easier to make it build little by little, so I'm just adding a few parts at a time from fsharp.psi to fsharp-new.psi, just to see how the compilation behaves.

Could you have a little look at my commit? It doesn't compile yet, I know how I can get it to compile but I'd like to be sure I'm not doing it wrong.

I made a few comments to give you more context.

  • I added this part in fsharp-new.psi: fsharp-new.psi
  • The problem is, the TokenType.cs generated file had errors because values on the right-hand of the assignment were undefined in FSharpTokenType (except for INT_LITERAL, which was manually defined).

generatedError

  • As such, I modified FSharpTokenType.cs so that right-hand parts are no more undefined values.
  • Additionally, I also modified the lexer FSharpLexer.gen.lex. I don't know if it was necessary? See here

Thanks again

@nesteruk
Owner

Wait a sec, those token types, shouldn't they be generated by TokenGenerator? Not sure we should be doing this by hand.

@OkayX6

Do you mean generated in FSharpTokenTypeGenerated.cs?
I think you confuse INT16_KEYWORD (ie int16) with INT16_LITERAL, the representation of a 16-bit integer: an integer literal with an 's' appended (ie. 125s), no?

like in the FSharpLexer.gen.lex file:
INT16_LITERAL = {INT_LITERAL}s

@OkayX6

If I don't define the INTxx_LITERAL variants in FSharpLexer.gen.lex and FSharpTokenType.cs, the lexer visualizer can't recognize them.

@OkayX6

I think I'm doing pretty well with the parts of the fsharp.psi grammar I've rewritten in fsharp-new.psi, but maybe I'm totally wrong :)!

I'd be glad to have your comments in my latest commit

My current depot has a parser that compiles fine even if Visual Studio 2012 doesn't say so, but turn solution-wide analysis and you'll see no errors.

As a better proof, just disable the parser generation (ParserGen) for fsharp-new.psi, as the tool deletes and then generates files, that user-defined files depend on (OperatorName.cs for instance).

@citizenmatt
Owner

Looking good. I've been going through the source to parserGen, and I'm getting close to being able to document the psi file format, and how the token/lexing/parsing code all hangs together.

As per the comment in the doc, the "private" keyword describes a private nonterminal - a node in the grammar that isn't added to the parse tree. Looking at the original fsharp.psi file, the intConstant node in the grammar is declared as private. So the generated parseIntConstant method doesn't create a CompositeElement, but simply adds a token to an existing element that gets passed in. In other words, intConstant is needed in the grammar, so that we can parse the file, but there's no intConstant type in the resulting tree - we simply add the appropriate INT_LITERAL terminal token to the parent CompositeElement.

@nesteruk
Owner

The great thing about having a fully functioning parser generator (even if incomplete) is that we get the ability to write lexer tests (this also solves #9). So maybe this is the way we should go - scrap the old .psi and re-add bits of new .psi at a time until we cover the whole of the F# language, all the while writing lexer and parser tests.

@OkayX6

@citizenmatt referring to the file history, the original fsharp.psi file did define intConstant as private, correct me if I'm wrong.

@nesteruk Do you mind then if I send you a pull request of the my current work?

@citizenmatt
Owner

Ooops. Yes. Didn't proof read! I've edited the comment so it makes sense.

@citizenmatt
Owner

I've just pushed a batch of changes that give us a clean compile for the psi file. Note that it compiles, but hasn't been tested, and I'd be very surprised if it actually works, but we can now get started on lexer tests.

Does this meet the definition of "fixed" enough to close the issue?

@nesteruk
Owner

I'm going to close this and spawn other issues as necessary.

@nesteruk nesteruk closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.