Skip to content

Add IsFabricatedToken assert#60005

Merged
cston merged 3 commits intodotnet:mainfrom
Youssef1313:fabricated-assert
Mar 15, 2022
Merged

Add IsFabricatedToken assert#60005
cston merged 3 commits intodotnet:mainfrom
Youssef1313:fabricated-assert

Conversation

@Youssef1313
Copy link
Copy Markdown
Member

@Youssef1313 Youssef1313 commented Mar 7, 2022 •
edited
Loading

Fixes #60002

@Youssef1313 Youssef1313 requested a review from a team as a code owner March 7, 2022 21:07
@ghost ghost added Community The pull request was submitted by a contributor who is not a Microsoft employee. Area-Compilers labels Mar 7, 2022
jcouv
jcouv reviewed Mar 7, 2022
View reviewed changes
src/Compilers/CSharp/Portable/Parser/Blender.Reader.cs Outdated
internal partial struct Blender
{
private struct Reader
#if DEBUG
Copy link
Copy Markdown
Member

@jcouv jcouv Mar 7, 2022

Choose a reason for hiding this comment

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

I'd go with simply internal

@jcouv
Copy link
Copy Markdown
Member

jcouv commented Mar 7, 2022

There's a build error in correctness leg:

D:\a\_work\1\s\src\Compilers\CSharp\Portable\Parser\LanguageParser.cs(4537,34): error CS0122: 'Blender.Reader' is inaccessible due to its protection level [D:\a\_work\1\s\src\Compilers\CSharp\Portable\Microsoft.CodeAnalysis.CSharp.csproj]
D:\a\_work\1\s\src\Compilers\CSharp\Portable\Parser\LanguageParser.cs(4537,34): error CS0122: 'Blender.Reader' is inaccessible due to its protection level [D:\a\_work\1\s\src\Compilers\CSharp\Portable\Microsoft.CodeAnalysis.CSharp.csproj]

@jcouv jcouv self-assigned this Mar 7, 2022
@RikkiGibson RikkiGibson mentioned this pull request Mar 7, 2022
Merged
@jcouv jcouv marked this pull request as draft March 8, 2022 08:10
@jcouv
Copy link
Copy Markdown
Member

jcouv commented Mar 8, 2022

I think this needs to be merged/rebased with latest main branch. Marked as draft in the meantime.
Ping when ready for review. Thanks

@Youssef1313
Copy link
Copy Markdown
Member Author

Youssef1313 commented Mar 8, 2022

I'm going to close per #60002 (comment). In case I was wrong and someone corrected me, I'll be happy to re-open and work on it.

@Youssef1313 Youssef1313 closed this Mar 8, 2022
@Youssef1313 Youssef1313 deleted the fabricated-assert branch March 8, 2022 21:05
@jcouv
Copy link
Copy Markdown
Member

jcouv commented Mar 8, 2022

Maybe I missed something in the other discussion. There are two questions:

  1. do we have an existing bug with incremental parsing of lambdas? (that was resolved)
  2. how can we remind ourselves to adjust IsFabricatedToken when new uses of MergeAdjacent are made?
    I thought the assertion would still be useful for the latter. Is that not the case?

Tagging @RikkiGibson @CyrusNajmabadi @cston

@Youssef1313
Copy link
Copy Markdown
Member Author

Youssef1313 commented Mar 8, 2022 •
edited
Loading

  • do we have an existing bug with incremental parsing of lambdas? (that was resolved)

My thought is "no we don't have an existing bug", but definitely better if someone else can take a look and confirm in case I'm wrong.

how can we remind ourselves to adjust IsFabricatedToken when new uses of MergeAdjacent are made?
I thought the assertion would still be useful for the latter. Is that not the case?

Maybe just a doc comment? Or an assert that we can skip for things where we think it's okay to not be included in IsFabricatedToken e.g. EqualsGreaterThanToken.

Note that #60002 is still open and should be tracking what the correct action is.

@jcouv
Copy link
Copy Markdown
Member

jcouv commented Mar 9, 2022

+1 on assert, as an assert would be a more effective guardrail than a comment.

@Youssef1313 Youssef1313 restored the fabricated-assert branch March 9, 2022 05:22
@Youssef1313 Youssef1313 reopened this Mar 9, 2022
@Youssef1313 Youssef1313 requested a review from jcouv March 9, 2022 05:53
@jcouv jcouv marked this pull request as ready for review March 9, 2022 07:10
jcouv
jcouv approved these changes Mar 9, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@jcouv jcouv left a comment

Choose a reason for hiding this comment

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

LGTM Thanks (iteration 3)

@Youssef1313
Copy link
Copy Markdown
Member Author

Youssef1313 commented Mar 15, 2022

@jcouv @cston Anything left to merge? Thanks!

@cston cston merged commit 0055818 into dotnet:main Mar 15, 2022
@ghost ghost added this to the Next milestone Mar 15, 2022
@cston
Copy link
Copy Markdown
Contributor

cston commented Mar 15, 2022

Thanks @Youssef1313.

@Youssef1313 Youssef1313 deleted the fabricated-assert branch March 15, 2022 15:06
@allisonchou allisonchou removed this from the Next milestone Mar 28, 2022
@allisonchou allisonchou added this to the 17.2.P3 milestone Mar 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jcouv jcouv jcouv approved these changes

+1 more reviewer

@cston cston cston approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@jcouv jcouv

Labels

Area-Compilers Community The pull request was submitted by a contributor who is not a Microsoft employee.

Projects

None yet

Milestone

17.2.P3

Development

Successfully merging this pull request may close these issues.

Verify incremental parsing merged EqualsGreaterThanToken in ParseLambdaExpression()

4 participants

@Youssef1313 @jcouv @cston @allisonchou