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

Improve error reporting: Missing = on type declaration #9642

Merged
merged 5 commits into from
Oct 13, 2020

Conversation

tboby
Copy link
Contributor

@tboby tboby commented Jul 7, 2020

This is my attempt to implement #1121, leaning on the start made in #6603.

This is my first contribution, so feedback appreciated!

I'm not quite sure what's happening with the xlf files. Is it possible a previous commit on master forgot to generate them?

@smoothdeveloper
Copy link
Contributor

@tboby

I'm not quite sure what's happening with the xlf files. Is it possible a previous commit on master forgot to generate them?

It is better to consider they are in .gitignore (despite they aren't), and not check them in unless explicitly asked during the review.

@abelbraaksma
Copy link
Contributor

abelbraaksma commented Jul 7, 2020

Not 100% sure, but maybe the xlf files had something to do with: #9641

I do know that they need to be rebuild when texts change, so that the translation team gets notified to translate the new texts. That that means that there's a bunch of changes at once seems to me to be expected.

@brettfo brettfo changed the base branch from master to main August 19, 2020 20:00
src/fsharp/pars.fsy Outdated Show resolved Hide resolved
src/fsharp/FSComp.txt Outdated Show resolved Hide resolved
@tboby tboby changed the title [WIP] Improve error reporting: Missing = on type declaration Improve error reporting: Missing = on type declaration Oct 11, 2020
@tboby
Copy link
Contributor Author

tboby commented Oct 11, 2020

I've:

  • rebased onto main
  • removed the xlf files as I wasn't sure if I was supposed to leave them in
  • updated the error message and parser naming as suggested

src/fsharp/FSComp.txt Outdated Show resolved Hide resolved
This is taken from the end of the list of idents. While the spec says this should be an ident not a long-ident, the parser doesn't enforce this, so we assume it has at least one item but don't check.
@tboby
Copy link
Contributor Author

tboby commented Oct 12, 2020

I've amended this to include the type name in the error message.
However, this required me to take the head or tail of the list of idents, which was odd, as the spec implies a type def's type name should be an ident:
type-name : attributesopt accessopt ident typar-defnsopt
not a long-ident (which includes dots).

The tyconDefn: parser definition I edit here eventually just uses path which is SyntaxTree.LongIdentWithDots. If you try and use dots in a type name you do seem to get an error, but this is caught by the TypeChecker which emits "Invalid type extension".

I may be missing something, but unless someone lets me know better I will write this up into an issue and consider having a go at changing this. I assume it is better to catch a spec syntax error in the parser than in the type checker?

@cartermp
Copy link
Contributor

I assume it is better to catch a spec syntax error in the parser than in the type checker?

Yes, absolutely :)

@dsyme any thoughts on the spec-y question?

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 for this work, it is great!

@cartermp cartermp merged commit 98dc1b0 into dotnet:main Oct 13, 2020
nosami pushed a commit to xamarin/visualfsharp that referenced this pull request Feb 23, 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.

5 participants