Detect whitespace in property name #5672

mfkl · 2020-08-23T12:27:03Z

Attempt at #5615

It is mentioned in MSBuild should error for invalid properties in conditional expression #5615 that MSBuild should both error and warn about this so not sure which is the correct approach here. An error might break a lot of existing code, but it might be the correct thing to do (?).
Also not sure whether this warrants a new string error message, though I did create one. If so, should I commit my untranslated xlf files? I am not familiar with the i18n process in MSBuild.

Any feedback welcome.

Forgind

Thanks for these changes! Would you mind making it more general by including all invalid characters rather than just whitespace?

src/Build.UnitTests/Scanner_Tests.cs

src/Build/Evaluation/Conditionals/Scanner.cs

src/Build/Resources/Strings.resx

Forgind · 2020-08-24T01:56:37Z

Looking at the error, you might need to target it more precisely when someone uses property functions. $([MSBuild]::ValueOrDefault('%(FullPath)', '')... is expected.

src/Build/Evaluation/Conditionals/Scanner.cs

mfkl · 2020-08-28T11:37:29Z

Thank you both for the feedback!

Taken most of your remarks into account, now working on

Looking at the error, you might need to target it more precisely when someone uses property functions. $([MSBuild]::ValueOrDefault('%(FullPath)', '')... is expected.

mfkl · 2020-08-28T14:28:27Z

Would you mind making it more general by including all invalid characters rather than just whitespace?

I have looked around a bit in docs and code, but I could not find such a list.

What would you consider invalid characters? https://docs.microsoft.com/en-us/visualstudio/msbuild/msbuild-special-characters?view=vs-2019?

Forgind · 2020-08-28T21:03:00Z

What would you consider invalid characters? https://docs.microsoft.com/en-us/visualstudio/msbuild/msbuild-special-characters?view=vs-2019?

@danmosemsft, is there a list somewhere of all the invalid characters?

rainersigwald · 2020-08-28T21:04:55Z

I think this is the check to use:

msbuild/src/Shared/XmlUtilities.cs

Lines 118 to 129 in 091189a

    
           /// <summary> 
        
           /// Indicates if the given name is valid as the name of an item, property or metadatum. 
        
           /// </summary> 
        
           /// <remarks> 
        
           /// Note that our restrictions are more stringent than those of the XML Standard. 
        
           /// </remarks> 
        
           /// <param name="name"></param> 
        
           /// <returns>true, if name is valid</returns> 
        
           internal static bool IsValidElementName(string name) 
        
           { 
        
               return LocateFirstInvalidElementNameCharacter(name) == -1; 
        
           }

mfkl · 2020-08-31T16:49:03Z

Hmmm IsValidSubsequentElementNameCharacter won't allow for expressions like

"'$(CSharpCoreTargetsPath)' == '' or ('$(DesignTimeBuild)' == 'true' and $(CSharpCoreTargetsPath.Contains('Microsoft.Net.Compilers.1.0.0')))"

It fails on the dot .

Forgind · 2020-08-31T17:10:06Z

Good point. Maybe exclude ., [, ], and :? I think those are the only relevant ones.

Unless you have nested properties/items, in which case also @, $, %, (, and )...

And also !?

mfkl · 2020-09-04T15:19:04Z

Including these extra valid character checks is failing a lot of tests, and the scope of the original PR is also quite larger than initially. There is an added perf consideration as well with this as mentioned in LocateFirstInvalidElementNameCharacter's comments, which is called a lot. I'm a bit unsure on how to proceed.

danmoseley · 2020-09-04T15:44:27Z

I would limit the fix to just whitespace since that’s the common mistake. I haven’t looked at the code recently, but there should be a place in the parser/scanner where encountering a close paren would complete a property name; in that place, it should reject space.

Forgind · 2020-09-18T16:45:06Z

We noticed that this only runs when looking at properties inside conditions. Would you mind making it work for all properties? After that, I think it's good.

mfkl · 2020-09-19T10:48:01Z

We noticed that this only runs when looking at properties inside conditions. Would you mind making it work for all properties?

Could you please share an example of such a property that I could use as a test case?

I may be wrong, but my understanding is that the whitespace check here is done from ParsePropertyOrItemMetadata which is called for all MSBuild properties? If so, IllFormedPropertyWhitespaceInCondition should probably be renamed.

stackedsax · 2020-09-25T17:23:07Z

@Forgind, any feedback for @mfkl on this one?

benvillalobos · 2020-09-29T02:30:10Z

This is supposed to throw an error when seeing whitespace in a property correct? I'm not seeing it (for properties inside or outside conditions), unless you have to build a specific way? I tried building a bootstrap of this PR and building this (taken from #5615):

<Project>
  <Target Name="t">
    <Warning Text="should print" Condition="'$(MSBuildProjectFullPath)' != ''"/>
    <Warning Text="should warn on this line" Condition="'$(MSBuildProjectFullPath )' != ''"/>
  </Target>
</Project>

Output when using standard msbuild and the bootstrapped msbuild are the same

Project "C:\src\repros\MSBuildPlayground\proj.csproj" on node 1 (default targets).
C:\src\repros\MSBuildPlayground\proj.csproj(6,5): warning : should print
Done Building Project "C:\src\repros\MSBuildPlayground\proj.csproj" (default targets).

No error is being thrown.

Same for building within a test:

        [Fact]
        public void MyTest()
        {

            string project = $"" +
                $"<Project>" +
                $"  <Target Name=\"Build\">" +
                $"    <Warning Text=\"should print\" Condition=\"'$(MSBuildProjectFullPath)' != ''\"/>" +
                $"    <Warning Text=\"should warn on this line\" Condition=\"'$(MSBuildProjectFullPath )' != ''\"/>" +
                $"  </Target>" +
                $"</Project>";

            MockLogger logger = ObjectModelHelpers.BuildProjectExpectSuccess(project);
            logger.AssertLogContains("MSB4151");
        }

Am I missing something?

benvillalobos · 2020-09-29T02:31:13Z

src/Build.UnitTests/Scanner_Tests.cs

+            AdvanceToScannerError(lexer);
+            Assert.Equal("IllFormedPropertyWhitespaceInCondition", lexer.GetErrorResource());
+
+            lexer = new Scanner("$( x)", ParserOptions.AllowProperties);


is it worth adding a test for "$(x yz" and "$(x y z")?

do you really mean "$(x yz" and "$(x y z") or actually "$(x yz)" and "$(x y z)"?

The latter 😊

mfkl · 2020-10-01T12:01:27Z

Thanks for your message.

This is supposed to throw an error when seeing whitespace in a property correct?

Yes.

I'm not seeing it (for properties inside or outside conditions), unless you have to build a specific way? I tried building a bootstrap of this PR and building this (taken from #5615):
No error is being thrown.

Indeed, I observe the same behavior with running msbuild.

Same for building within a test

For me it works when building within a unit test e624312, I can't explain yet why this test passes but msbuild.exe doesn't behave the same.
Building in diagnostics verbosity reveals

Task "Warning" skipped, due to false condition; ('$(MSBuildProjectFullPath )' != '') was evaluated as ('' != '').

Forgind · 2020-10-01T20:31:45Z

Interesting! So this is actually far more complicated than I'd imagined when I last commented here. Your unit test works unless you put quotes around the property, i.e.,

Condition=`'$(MSBuildProjectFullPath )' != ''`

Noting that there are also quotes around every instance in which @benvillalobos used a malformed property, I suspect that this doesn't look inside strings, and that there are two ways MSBuild handles properties. (Why? I have no idea.)

We may have to talk internally about what the best way forward is. I would say this PR is a clear improvement on what previously existed, but on the other hand, it doesn't cover all cases, and I don't know the easiest way to extend it to cover all cases. I imagine you could put something in Expander.cs to look at properties as they're expanded, but I don't think there's an easy way (or any way?) to get the position of the space in that case.

stackedsax · 2020-10-14T04:57:37Z

@Forgind any updates from your internal discussions about this one? Would love to hear about what you've discovered.

Forgind · 2020-10-16T17:12:01Z

After talking it over, we think it best to take it roughly as-is. We think it should go behind a change wave (talk to @benvillalobos if you need help with that), and we should change the error code to something that isn't taken, but otherwise, this is about the best we can do.

Does that sound reasonable to you, @mfkl?

Forgind · 2020-10-16T17:14:51Z

src/Build/Resources/Strings.resx

+    <value>MSB4151: Unexpected whitespace at position "{1}" of condition "{0}". Did you forget to remove a whitespace?</value>
+    <comment>{StrBegin="MSB4151: "}</comment>


Suggested change

<value>MSB4151: Unexpected whitespace at position "{1}" of condition "{0}". Did you forget to remove a whitespace?</value>

<comment>{StrBegin="MSB4151: "}</comment>

<value>MSB4259: Unexpected whitespace at position "{1}" of condition "{0}". Did you forget to remove a whitespace?</value>

<comment>{StrBegin="MSB4259: "}</comment>

(4151 was deprecated, but we should still avoid reusing error codes so people can search for them effectively online.)

benvillalobos · 2020-10-16T17:31:08Z

On change waves, here's a dev doc: https://github.com/dotnet/msbuild/blob/master/documentation/wiki/ChangeWaves-Dev.md any feedback on it would be appreciated!

This reverts commit e624312.

rename MSB4151 to MSB4259

mfkl · 2020-10-18T10:19:27Z

On change waves, here's a dev doc: https://github.com/dotnet/msbuild/blob/master/documentation/wiki/ChangeWaves-Dev.md any feedback on it would be appreciated!

Thanks!

I guess the doc is clear, though my initial reaction when reading it was "why was the opt-out strategy chosen over opt-in?".

I picked Wave17_0 a bit randomly. Maybe some guidance on how to choose whether to create a new Wave or use an existing one would be a good idea? I don't know, that may be tightly related to the feature itself (e.g. what are the chances it breaks customers builds?) and your way of versioning msbuild and roadmap, not sure that's relevant in that doc.

benvillalobos · 2020-10-20T20:19:24Z

Thanks for the feedback!

"why was the opt-out strategy chosen over opt-in?"

Fair question. Part of the drive for that is understanding the impact for some of these changes. If we hit a particularly hot path, it allows us to reevaluate the situation knowing that folks have a way out. It also gives us a better picture of how MSBuild is being used.

I picked Wave17_0 a bit randomly

Wave17_0 works. Note that this PR will have to wait a while to be merged until master becomes 17.0.

benvillalobos · 2020-10-20T20:20:40Z

src/Build.UnitTests/Scanner_Tests.cs

+            using TestEnvironment env = TestEnvironment.Create();
+            env.SetChangeWave(ChangeWaves.Wave17_0);
+
+            Scanner lexer = new Scanner("$(x )", ParserOptions.AllowProperties);
+            AdvanceToScannerError(lexer);
+            Assert.Null(lexer.UnexpectedlyFound);
+
+            lexer = new Scanner("$( x)", ParserOptions.AllowProperties);
+            AdvanceToScannerError(lexer);
+            Assert.Null(lexer.UnexpectedlyFound);


Suggested change

using TestEnvironment env = TestEnvironment.Create();

env.SetChangeWave(ChangeWaves.Wave17_0);

Scanner lexer = new Scanner("$(x )", ParserOptions.AllowProperties);

AdvanceToScannerError(lexer);

Assert.Null(lexer.UnexpectedlyFound);

lexer = new Scanner("$( x)", ParserOptions.AllowProperties);

AdvanceToScannerError(lexer);

Assert.Null(lexer.UnexpectedlyFound);

using (TestEnvironment env = TestEnvironment.Create())

{

env.SetChangeWave(ChangeWaves.Wave17_0);

Scanner lexer = new Scanner("$(x )", ParserOptions.AllowProperties);

AdvanceToScannerError(lexer);

Assert.Null(lexer.UnexpectedlyFound);

lexer = new Scanner("$( x)", ParserOptions.AllowProperties);

AdvanceToScannerError(lexer);

Assert.Null(lexer.UnexpectedlyFound);

}

Having TestEnvironment call Dispose resets state for CI builds.

The previous way actually calls Dispose, too, when env goes out of scope, and I think it's technically recommended, although it isn't something we use in MSBuild particularly.

I can change it back to the old syntax if you folks prefer.

So long as dispose is called I'm okay with its current state 👍

Forgind · 2020-10-20T20:51:18Z

I'd suggest moving to an earlier wave, probably 16.8 or 16.10.

We don't expect this to cause a problem 99% of the time, but someone theoretically could have properties with spaces that silently return empty strings and work around that, in which case this could break their build. If it were opt-in, we wouldn't find any customers like that, and we would never know whether there actually is anyone who relies on properties with spaces being turned into empty strings.

benvillalobos · 2020-10-20T21:55:27Z

I'd suggest moving to an earlier wave, probably 16.8 or 16.10.

Oh whoops, I meant 16.10 there. 16.8 works as well but we should decide soon. @marcpopMSFT @rainersigwald thoughts?

danmoseley · 2020-10-20T22:26:04Z

src/Build/Resources/Strings.resx

@@ -490,6 +490,10 @@
    <value>MSB4110: Expected a property at position {1} in condition "{0}". Did you forget the opening parenthesis after the '$'? To use a literal '$', use '%24' instead.</value>
    <comment>{StrBegin="MSB4110: "}</comment>
  </data>
+  <data name="IllFormedPropertyWhitespaceInCondition" xml:space="preserve">
+    <value>MSB4259: Unexpected whitespace at position "{1}" of condition "{0}". Did you forget to remove a whitespace?</value>


Nit, I don't think we normally would say a whitespace. Maybe a space. Suggest just ...forget to remove whitespace?

Forgind

LGTM! Thanks!

mfkl · 2020-10-27T04:27:06Z

I'm having trouble analyzing the CI result, I can't find what is failing exactly or why. Is it something I did?

Forgind · 2020-10-27T04:32:17Z

Doesn't look like it. I kicked off a new run. Since it was mono that failed, I'm guessing this flakiness will be fixed by #5831.

rainersigwald · 2020-11-04T17:19:53Z

Thanks, @mfkl!

benvillalobos · 2020-11-04T17:20:09Z

Thanks for the change @mfkl

Forgind reviewed Aug 24, 2020

View reviewed changes

src/Build.UnitTests/Scanner_Tests.cs Outdated Show resolved Hide resolved

src/Build/Evaluation/Conditionals/Scanner.cs Outdated Show resolved Hide resolved

src/Build/Resources/Strings.resx Outdated Show resolved Hide resolved

cdmihai reviewed Aug 28, 2020

View reviewed changes

src/Build/Evaluation/Conditionals/Scanner.cs Outdated Show resolved Hide resolved

cdmihai approved these changes Aug 28, 2020

View reviewed changes

mfkl force-pushed the bugfix/5615 branch from 10fa40d to 5e653c1 Compare August 31, 2020 16:19

mfkl force-pushed the bugfix/5615 branch from 2263488 to 652c4ed Compare September 6, 2020 18:27

benvillalobos reviewed Sep 29, 2020

View reviewed changes

Forgind reviewed Oct 16, 2020

View reviewed changes

mfkl added 5 commits October 18, 2020 16:20

Detect whitespace in property name

cee8889

try adding xlf files

529db6b

remove extra line

e146514

review feedback

062404a

update xlf files

d209d1e

mfkl and others added 5 commits October 18, 2020 16:20

only check for whitespace inside properties

6bb1424

Parser tests: Add whitespace test

20297ad

Revert "Parser tests: Add whitespace test"

359bafc

This reverts commit e624312.

don't reuse deprecated MSBuild error codes

d418a4a

rename MSB4151 to MSB4259

Use ChangeWaves for whitespace check

e5ce9e3

mfkl force-pushed the bugfix/5615 branch from e624312 to e5ce9e3 Compare October 18, 2020 10:01

benvillalobos reviewed Oct 20, 2020

View reviewed changes

danmoseley reviewed Oct 20, 2020

View reviewed changes

mfkl added 2 commits October 24, 2020 14:15

rename whitespace to space

31ddd02

Use wave 16.10

8132520

Forgind approved these changes Oct 24, 2020

View reviewed changes

rainersigwald approved these changes Nov 3, 2020

View reviewed changes

benvillalobos merged commit 0ad5f83 into dotnet:master Nov 4, 2020

mfkl deleted the bugfix/5615 branch November 5, 2020 05:54

jonathanpeppers mentioned this pull request Nov 5, 2020

.NET 6: error MSB4259: Unexpected space at position N Did you forget to remove a space? #5863

Closed

		<value>MSB4151: Unexpected whitespace at position "{1}" of condition "{0}". Did you forget to remove a whitespace?</value>
		<comment>{StrBegin="MSB4151: "}</comment>

Detect whitespace in property name #5672

Detect whitespace in property name #5672

Conversation

mfkl commented Aug 23, 2020

Forgind left a comment

Choose a reason for hiding this comment

Forgind commented Aug 24, 2020

mfkl commented Aug 28, 2020

mfkl commented Aug 28, 2020

Forgind commented Aug 28, 2020

rainersigwald commented Aug 28, 2020

mfkl commented Aug 31, 2020

Forgind commented Aug 31, 2020 • edited Loading

mfkl commented Sep 4, 2020

danmoseley commented Sep 4, 2020

Forgind commented Sep 18, 2020

mfkl commented Sep 19, 2020

stackedsax commented Sep 25, 2020

benvillalobos commented Sep 29, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mfkl commented Oct 1, 2020

Forgind commented Oct 1, 2020

stackedsax commented Oct 14, 2020

Forgind commented Oct 16, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benvillalobos commented Oct 16, 2020

mfkl commented Oct 18, 2020

benvillalobos commented Oct 20, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Forgind commented Oct 20, 2020

benvillalobos commented Oct 20, 2020

Choose a reason for hiding this comment

Forgind left a comment

Choose a reason for hiding this comment

mfkl commented Oct 27, 2020

Forgind commented Oct 27, 2020

rainersigwald commented Nov 4, 2020

benvillalobos commented Nov 4, 2020

Forgind commented Aug 31, 2020 •

edited

Loading

benvillalobos commented Oct 20, 2020 •

edited

Loading