OnlyCheckWhitespaceInsideParenthesis Fixes #5863 #5868
Conversation
This weakens the check somewhat, since it only checks just inside parentheses, but that allows people to put spaces in, say, property conditions after a comma.
| @@ -333,17 +331,20 @@ private static bool ScanForPropertyExpressionEnd(string expression, int index, o | |||
| if (character == '(') | |||
| { | |||
| nestLevel++; | |||
| whitespaceCheck = true; | |||
| if (index + 1 < expression.Length && char.IsWhiteSpace(pchar[index + 1]) && ChangeWaves.AreFeaturesEnabled(ChangeWaves.Wave16_10)) | |||
There was a problem hiding this comment.
we only care about a space after an open parens and before a close parens? What about before / after each one? That's the case here: #5863
<ItemGroup Condition="$(Foo.StartsWith ('A'))">
There was a problem hiding this comment.
Right, so we should ignore cases like that because we don't want them to fail.
| @@ -325,33 +325,35 @@ private static bool ScanForPropertyExpressionEnd(string expression, int index, o | |||
| { | |||
| fixed (char* pchar = expression) | |||
| { | |||
| if (expression.Length > 1 && pchar[0] == '(' && char.IsWhiteSpace(pchar[1]) && ChangeWaves.AreFeaturesEnabled(ChangeWaves.Wave16_10)) | |||
There was a problem hiding this comment.
is the first character in the expression guaranteed to be a '('?
Should this maybe move under the `if (character == '(')?
Something like:
if (character == '(')
{
if (nestLevel == 0 && index + 1 > expression.Length && pchar[index + 1] == '(' && char.IsWhiteSpace(pchar[1]) && ChangeWaves.AreFeaturesEnabled(ChangeWaves.Wave16_10))
{
indexResult = 1;
return false;
}
nestLevel++;
}
src/Build.UnitTests/Scanner_Tests.cs
Outdated
| [InlineData("$(x x)")] | ||
| [InlineData("$(y d f ( hi ) sx)")] |
There was a problem hiding this comment.
These are examples of things we wish we could catch but we can't with current limitations. Can you instead use examples we want to work like
| [InlineData("$(x x)")] | |
| [InlineData("$(y d f ( hi ) sx)")] | |
| [InlineData("$(x.StartsWith( 'y' ))")] | |
| [InlineData("$(x.StartsWith ('y'))")] |
?
There was a problem hiding this comment.
Actually, looking at this, I think even starts/ends with space is too aggressive. $( x.StartsWith ( y ) ) is a bit ugly but works just fine today. Maybe it has to be "has a space and is otherwise only identifier characters"? That would correctly reject $(x x) from your examples here.
Co-authored-by: Rainer Sigwald <[email protected]>
| [InlineData("$(x.StartsWith( 'y' ))")] | ||
| [InlineData("$(x.StartsWith ('y'))")] | ||
| [InlineData("$( x.StartsWith( $(SpacelessProperty) ) )")] | ||
| [InlineData("$( x.StartsWith( $(_SpacelessProperty) ) )")] |
There was a problem hiding this comment.
One case I ran into was something like:
[InlineData("$(x.StartsWith('Foo', StringComparison.InvariantCultureIgnoreCase))")]It might already work with the changes here, but might be good to add this case.
There was a problem hiding this comment.
Do please add this multiple-argument case before merging.
| [InlineData("$(x.StartsWith( 'y' ))")] | ||
| [InlineData("$(x.StartsWith ('y'))")] | ||
| [InlineData("$( x.StartsWith( $(SpacelessProperty) ) )")] | ||
| [InlineData("$( x.StartsWith( $(_SpacelessProperty) ) )")] |
There was a problem hiding this comment.
Do please add this multiple-argument case before merging.
This weakens the check somewhat, since it only checks just inside parentheses, but that allows people to put spaces in, say, property conditions after a comma.
Fixes #5863