Skip to content

Conversation

@abelbraaksma
Copy link
Contributor

@abelbraaksma abelbraaksma commented Aug 9, 2020

Fixes #3487.
Fixes #9787.

[continued from #9779, because somehow Github didn't understand the diff-view anymore, showing 100s unrelated non-existing changes. Close/recreate PR with exactly the same changes seemed to fix this]

This resurrects the work done in #5498 by @misonijnik . In short, it's an attempt to make sure that:

#iff FALSE   // raises wrong error, this should fix that (should be invalid token error)
#foo "test" // doesn't raise error at all, but should (invalid token)
#foo 42   // raises wrong error, this should fix that (should be invalid token error)

module X =
    #foo 42   // shows warning, but should be error (invalid token)

Some tests were failing on the original PR, I merged it with latest master and marked this WIP, as I want to see what those errors were, though I don't expect to work long on it, the change is pretty benign ;).

List of fixes (will be updated if necessary)

  • Fix wrong hash-token parsing (partially done)

    • with the original change by @misonijnik, # is now only recognized and leading to tokens if it is valid. This should be updated to include invalid items and the proper error and range for the lang service.
  • Allow a type to start with the letters "if"

    Previously
    image

    Implemented:
    image

  • Fix wrong error for use of #indent (text mentions '#light', instead of '#indent') (FS0000 warning (not assigned) for #light or #indent being misplaced #9787):
    image

    Implemented, whole token shown:
    image

  • Fix wrong error for use of #light (FS0000 warning (not assigned) for #light or #indent being misplaced #9787):
    image

    Implemented:
    image

  • Fix wrong error when using wrong argument with #light:
    image

  • Improve #light when token is wrong:
    image

    After @misonijnik's change (error and range still to improve):
    image

  • Fix wrong error with wrong token with #if:
    image

    After @misonijnik's change (error and range still to improve):
    image

  • This fsharpqa test explicitly tests for leniency with hash-types starting with #light: Conformance\TypesAndTypeConstraints\TypeParameterDefinitions (HashConstraint02.fs).

    // Regression test for FSHARP1.0:1419
    // Tokens beginning with # should not match greedily with directives
    // The only case where we are still a bit confused is #light: for this reason the code
    // below compiles just fine (it would not work if I replace #light with #r for example)
    #light
    
    type light_() = class
                    end
    
    let t = new light_()
    
    let t5 (x : #light_) = x       // this is now valid, and does not throw a warning
    
    let r1 = t5 1.0         // this is now INVALID! (as it should)
    let r2 = t5 t
    
    exit 0
    

    Current situation
    image

    New situation
    image

  • Fix wrong parsing of light when used as hash-constraint:

    Current situation
    image

Generally: when # is followed by an unrecognized token, I would expect an error that says that the token is not recognized.

TODO:

  • Fix failing build
  • Verify & improve errors / warnings
  • add tests
  • Review / improve code changes

@cartermp
Copy link
Contributor

@abelbraaksma for the sake of incrementality, how would you feel if we just reviewed and aimed to merge what is here already?

@abelbraaksma
Copy link
Contributor Author

@cartermp, that's probably a good idea. Let me merge and fix over the weekend so that this can be reviewed.

Copy link
Contributor

@cartermp cartermp left a comment

Choose a reason for hiding this comment

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

Thanks! I think this looks good.

@cartermp cartermp changed the title [WIP] Fix lexing hash directives and raise errors (2nd attempt) Fix lexing hash directives and raise errors (2nd attempt) - several cases Feb 24, 2021
Copy link
Contributor

@KevinRansom KevinRansom left a comment

Choose a reason for hiding this comment

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

Looks, good, but what do you want to do with the todo?

// TODO unreachable error above, I think? - brianmcn
let s = lexeme lexbuf
warning(Error((0, sprintf "%s should only be set once in an F# source file." s), lexbuf.LexemeRange))
// TODO: where should this go? (abelb)
Copy link
Contributor

Choose a reason for hiding this comment

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

@abelbraaksma --- is this intended?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@KevinRansom I think I meant that all errors go in a resource file (iirc) and this line as it is is not localizable. When I wrote that I didn't know where to place the localizable string.

The other thing here is probably the error/warning number, which here is 0. I don't know what number to choose. It's been like that for a while, so I guess I could also fix it in a future pr. How are these numbers assigned? Just choose a free one?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Side effect of current code is, I guess, that you won't be able to disable this warning as it is written...

@cartermp
Copy link
Contributor

cartermp commented Mar 3, 2021

I will merge this since the comment is benign.

How are these numbers assigned? Just choose a free one?

Yep, you can create a new one in FSComp.txt and when you run build it will generate the necessary resource files.

@cartermp cartermp merged commit 261f5dc into dotnet:main Mar 3, 2021
@cartermp
Copy link
Contributor

cartermp commented Mar 3, 2021

Thanks for the contribution! Feel free to follow up as you see fit.

@dsyme
Copy link
Contributor

dsyme commented Mar 3, 2021

This one caused a build break on integration

/home/vsts/work/1/s/src/fsharp/lex.fsl(984,50): error FS0039: The type 'LexBuffer<Char>' does not define the field, constructor or member 'Lexeme'. Maybe you want one of the following:�   LexemeView�   LexemeChar�   LexemeRange�   LexemeString�   LexemeContains [/home/vsts/work/1/s/src/fsharp/FSharp.Compiler.Service/FSharp.Compiler.Service.fsproj]
##[error]src/fsharp/lex.fsl(984,50): error FS0039: The type 'LexBuffer<Char>' does not define the field, constructor or member 'Lexeme'. Maybe you want one of the following:�   LexemeView�   LexemeChar�   LexemeRange�   LexemeString�   LexemeContains

@dsyme
Copy link
Contributor

dsyme commented Mar 3, 2021

Changing

{ let n = Array.IndexOf(lexbuf.Lexeme, '#')

to

{ let n = (lexeme lexbuf).IndexOf('#')

worked for me

I'll submit a PR

@dsyme dsyme mentioned this pull request Mar 3, 2021
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.

FS0000 warning (not assigned) for #light or #indent being misplaced Invalid directives are not a syntax error, unless in specific places

5 participants