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

Update syntax document to better indicate when we expect a list to have at least one element in it. #37680

Merged

Conversation

CyrusNajmabadi
Copy link
Member

@CyrusNajmabadi CyrusNajmabadi commented Aug 2, 2019

Tagging @gafter . This helps with a piece of work i'm trying to contribute to roslyn where we can generate an g4 grammar like so from our syntax model: https://gist.github.com/CyrusNajmabadi/412c3209d1ce97236420218498e7c8d4

While not strictly necessary, this helps me generate cleaner and clearer code since we can replace stuff like:

attribute_list
  : '[' attribute_target_specifier? (attribute (',' attribute)*)? ']'
  ;

With the nicer, simpler and clearer:

attribute_list
  : '[' attribute_target_specifier? attribute (',' attribute)* ']'

Similarly, AllowTrailingSeparator means we can generate:

initializer_expression
  : '{' (expression (',' expression)* ','?)? '}'
  ;

Which is accurate, unlike:

initializer_expression
  : '{' (expression (',' expression)*)? '}'
  ;

Which is not (since it implies that var v = { 1, 2, 3, } is not legal. We could a trailing optional comma in all productions. But that wouldn't be very nice or representative of what the language actually thinks.

@CyrusNajmabadi CyrusNajmabadi requested a review from a team as a code owner August 2, 2019 19:55
@CyrusNajmabadi
Copy link
Member Author

CyrusNajmabadi commented Aug 2, 2019

Tagging @jcouv as well. Note, this is part of a quartet of PRs i intend here (no more than that). The PRs are:

  1. Make anonymous-method's body strongly typed: Switch to a strongly typed version of anonymous-function syntax nodes for their "body" #37674
  2. Make it clear when a list is 1-or-more, versus 0-or-more (this PR).
  3. Make it clear when fields are exclusive of each other. Make it clear in Syntax.xml when fields are exclusive of each other. #37684
  4. Make it clear when a separate syntax list allows a trailing separator.

I don't intend to do anything beyond that (besides contribute my tool to generate the g4 grammar from our Syntax.xml).

This was inspired from the following customer request here: dotnet/csharplang#2640

I would like us to have one 'source of truth' in the codebase. So if we are to have another grammar doc written, i think it would be nice if it was 100% in sync with our syntax.xml descriptions.

@CyrusNajmabadi
Copy link
Member Author

@tmat something seems to be going very badly with EnC. This is a change that had no code impact, but all the Visual Basic enc tests are failing with stuff like:

Errors Microsoft.VisualStudio.LanguageServices.IntegrationTests.dll
    Roslyn.VisualStudio.IntegrationTests.VisualBasic.BasicEditAndContinue.LocalsWindowUpdatesAfterLocalGetsItsTypeUpdatedDuringEnC [FAIL]
    Roslyn.VisualStudio.IntegrationTests.VisualBasic.BasicEditAndContinue.EnCWhileDebuggingFromImmediateWindow [FAIL]
    Roslyn.VisualStudio.IntegrationTests.VisualBasic.BasicEditAndContinue.LocalsWindowUpdatesCorrectlyDuringEnC [FAIL]
    Roslyn.VisualStudio.IntegrationTests.VisualBasic.BasicEditAndContinue.WatchWindowUpdatesCorrectlyDuringEnC [FAIL]
    Roslyn.VisualStudio.IntegrationTests.VisualBasic.BasicEditAndContinue.UpdateActiveStatementLeafNode [FAIL]
    Roslyn.VisualStudio.IntegrationTests.VisualBasic.BasicEditAndContinue.DocumentStateTrackingReadonlyInRunMode [FAIL]
Command: C:\Users\vsagent\.nuget\packages\xunit.runner.console\2.4.1-pre.build.4059\tools\net472\xunit.console.x86.exe "F:\workspace\_work\1\s\artifacts\bin\Microsoft.VisualStudio.LanguageServices.IntegrationTests\Debug\net472\Microsoft.VisualStudio.LanguageServices.IntegrationTests.dll"  -xml "F:\workspace\_work\1\s\artifacts\TestResults\Debug\Microsoft.VisualStudio.LanguageServices.IntegrationTests.dll_net472_x86.xml" -noshadow -verbose
xUnit output log: F:\workspace\_work\1\s\artifacts\log\Debug\xUnitFailure-Microsoft.VisualStudio.LanguageServices.IntegrationTests.dll.log
    Roslyn.VisualStudio.IntegrationTests.VisualBasic.BasicEditAndContinue.LocalsWindowUpdatesAfterLocalGetsItsTypeUpdatedDuringEnC [FAIL]
    Roslyn.VisualStudio.IntegrationTests.VisualBasic.BasicEditAndContinue.EnCWhileDebuggingFromImmediateWindow [FAIL]
    Roslyn.VisualStudio.IntegrationTests.VisualBasic.BasicEditAndContinue.LocalsWindowUpdatesCorrectlyDuringEnC [FAIL]
    Roslyn.VisualStudio.IntegrationTests.VisualBasic.BasicEditAndContinue.WatchWindowUpdatesCorrectlyDuringEnC [FAIL]
    Roslyn.VisualStudio.IntegrationTests.VisualBasic.BasicEditAndContinue.UpdateActiveStatementLeafNode [FAIL]
    Roslyn.VisualStudio.IntegrationTests.VisualBasic.BasicEditAndContinue.DocumentStateTrackingReadonlyInRunMode [FAIL]

Do you know anything about this?

@tmat
Copy link
Member

tmat commented Aug 2, 2019

@sharwell @ivanbasov I don't see any obvious issue.

@ivanbasov
Copy link
Contributor

I can assume that tests are broken in the current pipeline (https://dev.azure.com/dnceng/public/_build/results?buildId=292810&view=ms.vss-test-web.build-test-results-tab&runId=8258028&resultId=100005&paneView=history) but cannot understand since when they become enabled again. I remember they were disabled a couple months ago.

@sharwell and @JoeRobich may know more.

@JoeRobich
Copy link
Member

These are the same set of tests that were failing last month. I have a pr where I reverted my change that fixed the earlier problem. maybe the images have been rebuilt with the new preview and we are back in a better place again. #37686

@CyrusNajmabadi CyrusNajmabadi changed the base branch from master to release/dev16.4-preview1 August 6, 2019 08:39
@CyrusNajmabadi
Copy link
Member Author

Tagging @gafter . This is a syntax.xml only change purely to add some useful grammatic hints to other downstream tooling.

@CyrusNajmabadi
Copy link
Member Author

@RikkiGibson can you ptal? A very simple cahnge to just help downstream tools.

@jinujoseph jinujoseph added Area-Compilers Community The pull request was submitted by a contributor who is not a Microsoft employee. labels Aug 6, 2019
@333fred
Copy link
Member

333fred commented Aug 6, 2019

While I don't have any real issue with adding this info, I'm concerned that there's no verification of these attributes. What's the stop new syntax forms needing to have these added, for example? I'm just concerned that these are just an even less visible form of stale comments.

@CyrusNajmabadi
Copy link
Member Author

As per discussion with @333fred . My goal is to literally have roslyn include (as part of the normla syntax generators) code that produces: https://gist.github.com/CyrusNajmabadi/412c3209d1ce97236420218498e7c8d4

This way we can just have a reasonable .g4 grammar checked in that both the team and external users can refer to as an easy way to get an overview of the grammar of at least the default roslyn impl syntax-model/parser. Note: this could definitely tell about programs that not be allowed (because they didn't fit into this grammar). But, like all grammars, it wouldn't tell you the entire set of programs that would be allowed. i.e. code might be legal under the grammar, but would still be rejected by roslyn for any number of additional rules we apply on top of the ones codified in the grammar itself.

@gafter
Copy link
Member

gafter commented Aug 6, 2019

@CyrusNajmabadi I don't think Syntax.xml can ever be used to generate a correct grammar, because it is intentionally a generalization of the real language grammar. For example, how are you going to handle precedence? Still, I think this is a worthwhile exercise.

Copy link
Member

@gafter gafter left a comment

Choose a reason for hiding this comment

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

:shipit:

@CyrusNajmabadi
Copy link
Member Author

@CyrusNajmabadi I don't think Syntax.xml can ever be used to generate a correct grammar, because it is intentionally a generalization of the real language grammar.

Being pedantic, a grammar without precedence is fine. If it rejects a program, that program could never be accepted by the actual C# langauge or parser. It may accept programs that are not legal programs, but that's ok, that's just like normal grammars.

I agree this isn't as tight a grammar specification as some might like. But i think that's an ok place to start with.

For example, how are you going to handle precedence?

Simple, like i did in my compilers class. I had my language spec list a table of precedence, and the rest of my system which consumed the unprecedented (no pun intended) tree, then rewrote it to be in precedence form :)

Note: without question, this is an ambiguous grammar. if you have a + b + c it says there are multiple legals ways to parse that. That's fine IMO. It can be defined 'elsewhere' (for some broad defnition of elsewhere) which of theses legal parses are considered acceptable.

As above, it woudl certainly be nice to encode as much as possible into the grammar to not need as much text 'elsewhere'. But i'm ok with this :D

@CyrusNajmabadi
Copy link
Member Author

@RikkiGibson @gafter @333fred Feel free to merge. Thanks! :) Only one PR to go :)

@RikkiGibson RikkiGibson merged commit d7fe127 into dotnet:release/dev16.4-preview1 Aug 7, 2019
@CyrusNajmabadi CyrusNajmabadi deleted the oneOrMore branch January 25, 2020 00:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Compilers Community The pull request was submitted by a contributor who is not a Microsoft employee.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants