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

Fix bug when trailing and leading underscores are not the same #22141

Merged
merged 1 commit into from
Sep 27, 2017

Conversation

alrz
Copy link
Member

@alrz alrz commented Sep 15, 2017

Report 0x_2_ as an invalid number.

(VB does report it correctly)

@@ -1198,7 +1196,10 @@ private bool ScanNumericLiteral(ref TokenInfo info)
default:
if (string.IsNullOrEmpty(valueText))
{
this.AddError(MakeError(ErrorCode.ERR_InvalidNumber));
if (!underscoreInWrongPlace)
Copy link
Member Author

@alrz alrz Sep 15, 2017

Choose a reason for hiding this comment

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

If we don't do this 0x_ produces two errors. (is there any way to handle it somewhere else?)

@alrz
Copy link
Member Author

alrz commented Sep 15, 2017

@jcouv @cston for review.

@sharwell sharwell added the Community The pull request was submitted by a contributor who is not a Microsoft employee. label Sep 15, 2017
@@ -2815,6 +2815,17 @@ public void TestNumericWithBadUnderscores()
Assert.Equal("error CS1013: Invalid number", errors[0].ToString(EnsureEnglishUICulture.PreferredOrNull));
Assert.Equal(text, token.Text);

text = "0x_2_";
token = LexToken(text, _options72);
Copy link
Member

@cston cston Sep 18, 2017

Choose a reason for hiding this comment

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

Please add a corresponding test for VB. #Resolved

Copy link
Member Author

@alrz alrz Sep 20, 2017

Choose a reason for hiding this comment

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

Added. #Resolved

@jcouv jcouv added this to the 15.5 milestone Sep 18, 2017
if (usedUnderscore)
else if (firstCharWasUnderscore)
{
CheckFeatureAvailability(MessageID.IDS_FeatureLeadingDigitSeparator);
Copy link
Member

Choose a reason for hiding this comment

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

Consider moving the new CheckFeatureAvailability call into the hex/binary condition.
Or, if this call can affect other scenarios than hex/binary, then let's ensure those code paths have test coverage too. Thanks

Copy link
Member Author

@alrz alrz Sep 20, 2017

Choose a reason for hiding this comment

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

Then we would still need to check !firstCharWasUnderscore to not get cascading feature errors.

Copy link
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

@gafter gafter self-assigned this Sep 27, 2017
@gafter
Copy link
Member

gafter commented Sep 27, 2017

test windows_debug_vs-integration_prtest please

@gafter
Copy link
Member

gafter commented Sep 27, 2017

test windows_release_vs-integration_prtest please

@gafter
Copy link
Member

gafter commented Sep 27, 2017

@jcouv Can you please approve for 15.5? This is fixing a regression that was introduced when implementing the "permit leading underscore after base specifier" feature. The current code base permits 0x_2_. If we don't fix it in 15.5 then we'll have to introduce a breaking change to fix it later.

@jcouv
Copy link
Member

jcouv commented Sep 27, 2017

Approved for ask mode

@jcouv jcouv merged commit bbf380b into dotnet:master Sep 27, 2017
@alrz alrz deleted the trailing-underscore branch September 27, 2017 16:41
333fred added a commit to 333fred/roslyn that referenced this pull request Sep 28, 2017
* dotnet/master: (35 commits)
  removed unused options (dotnet#22389)
  Make readonlyness of ref-ternary the AND of operands (dotnet#22384)
  Refine parsing look-ahead for type arguments (dotnet#22102)
  IDE Features fix for code generation for ref-readonly (dotnet#22285)
  Added tests for `out` and `ref` completion in local function
  Record "allow digit separator after 0b or 0x" (dotnet#22371)
  Responded to feedback
  Don't show snippets in VB comments
  Fix bug when trailing and leading underscores are not the same (dotnet#22141)
  Add breaking change doc for TypedReference delegate conversion. (dotnet#22084)
  Add no-PDB-path test for VB
  Ref ternary shoudl be shielded form optimization in the containing expression that may expose it to mutations
  Improve method type argument inference around tuples and address backwards compatibility break. (dotnet#22295)
  Added couple more tests
  Add unit tests for IStopStatement and IEndStatement and make the APIs public again (dotnet#22225)
  Revert changes to compilers.sln
  Support for object initializers.
  supports all expressions in escape analysis that can possibly produce ref-like values
  Update tests after merge.
  Add an option to disable name suggestions
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Compilers cla-already-signed 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.

6 participants