Fix NRE introduced in FileSystemWatcher during nullability annotations

[krwq]

While annotating #41261 I've introduced a product bug which luckily got caught by tests. It is weird compiler behavior (dotnet/csharplang#3393) which changes ?. precedence when adding !, see i.e.:
https://sharplab.io/#v2:EYLgtghgzgLgpgJwDQBMQGoA+ABATARgFgAoEvAZgAIBvEy+y7fANkYBZKAFCGAC3wAUAYXyUADgEoadBrIBuEBJQB2EMHEoBecQH4AdENx6AcmrgBuGbPpMAnANXqJl4rIC+Vyp6atsHbny4wqKS0q7W9ApKjhraYvqGAIQmZi4RNvj2Mc6eHsR5ZLiUIiS04TZUhsVF1JQA5nAw5pRQjc15BcR41aXeVEwADJSm6jT1bS1tHUA

I've done quick search in our code if we've introduced similar errors elsewhere and found only this.

Note: this is fixing a regression introduced by e59c926#diff-ab050074db1f148b9d552c569a7899b7R398.

[danmoseley]

Ouch. Nasty compiler bug. Can you add a test?

[danmoseley]

I've done quick search in our code if we've introduced similar errors elsewhere and found only this.

How did you do such a search? Look for "?." followed by "!." ? I am wondering how reliable a check this is and whether we ought to ask for an instrumented compiler to help find this cas.e

[jeffhandley]

Please apply code comments similar to my suggestions; otherwise looks good to me.

[krwq]

@danmosemsft

How did you do such a search?

RegEx, yes ?. followed by !. (only single line searches), perhaps we could do so but IMO there will be relatively small chance of that happening (I was expecting more occurrences with RegExes), this would be ideal if we have cycles for that - I personally am not too familiar with how to search for that using Roslyn and also expecting there will be no more entries but if someone knows how to do it quickly I recommend we should do it (perhaps even better might be to ask compiler team to fix this problem rather than investing in workarounds, especially I've seen suggested code fix in one of the compiler issues).

Can you add a test?

I'll see if I can find some easy way to repro but considering this is affecting Linux only and possibly some edge case I'm not sure how hard this will be so I'll time box the research.

[danmoseley]

@agocke any suggestions for how we might find any other instances of this insidious error? Beyond the regex for \?\.\w+!\. which @krwq already performed

[krwq]

@danmosemsft I actually search twice per line so I don't miss cases like foo?.bar.baz!.foo2; Didn't want to mess with Regexes too much as it's easy to make stupid mistake.

Here is the code I used

staticreadonlyRegexs_r1=newRegex(@"[a-z0-9_]\?\.",RegexOptions.IgnoreCase|RegexOptions.Compiled);staticreadonlyRegexs_r2=newRegex(@"[a-z0-9_]\!\.",RegexOptions.IgnoreCase|RegexOptions.Compiled);staticboolIsBuggyCompilerCase(stringline){Matchm1=s_r1.Match(line);if(m1.Success){intstart=m1.Index;foreach(Matchm2ins_r2.Matches(line)){intend=m2.Index;if(start<end){returntrue;}}}returnfalse;}

and searching in all directories for all .cs files

[krwq]

@danmosemsft I've tried several ways to repro this including with symlinks and other combinations of removing/renaming (as suggested per comment) but I'm unsure how to do it or if it's even reproducible. Probably owners might have some clues or ideas (cc: @maryamariyan@carlossanlop). Looking at the code it's related to existing file handle which doesn't have a parent which suggests a root of FS Watcher (Path property) - I've also tried removing the root but also haven't seen anything which would manifest as any sort of NRE.. I'd still do the fix just in case but not 100% sure if worth porting unless we understand what might possibly be broken

[carlossanlop]

Does this need to be backported anywhere?

[carlossanlop]

@krwq can you try this regex?

\w(\([\w\s,]*\))?[!?]\.+(\w(\([\w\s,]*\))?[!?]\.)+

It's not as thorough as Roslyn could be, but at least it can find a few more instances. For example, it worked with this code:

staticvoidMain(){A?.B.D;// NoA!.B.C!.D;// NoA()!.B().C()!.D;// NoA(a,b)!.B(b,c).C(c,d)?.D;// NoA?.B!.C;// YesA?.B!.C!.D;// YesA!.B?.C!.D?.E;// YesA()!.B()?.C().D;// YesA(a)!.B(b)?.C().D;// YesA(a,b)!.B(b,c)!.C.D;// Yes}

Please correct me if any of the cases is wrong.

[danmoseley]

It's OK that you can't figure out how to test it. That happens. The change clearly returns us to previous behavior, and your sharplab.io demonstrates why we need to do that. Please port into 5.0.

Any further regexing throws up another instance, please do follupw fix.

[danmoseley]

Oh I can trigger it

/backport to release/5.0

[danmoseley]

/backport to release/5.0

[github-actions]

Started backporting to release/5.0: https://github.com/dotnet/runtime/actions/runs/224080391

[github-actions]

Started backporting to release/5.0: https://github.com/dotnet/runtime/actions/runs/224080752

[danmoseley]

Oops.. let me close one..

[danmoseley]

@akoeplinger just curious, how did the bot not end up creating two PR's here?

[akoeplinger]

@danmosemsft before opening a PR it checks whether the branch already exists on the origin remote to avoid opening another PR. This allows you to push changes to the source PR and do another /backport and the changes will be pushed to the existing backport branch/PR.

[krwq]

@carlossanlop I wasn't able to find anything new with your Regex directly but your tests gave me idea of one important case I missed (calls in the chain) and I found one more.

[carlossanlop]

@danmosemsft is your comment going to do the port automatically? I ask because I have this PR open to port all the nullability changes manually together, so I'd like to know if I should skip this one.

[carlossanlop]

@danmosemsft confirmed, this change is already merged to release/5.0: 2aa627b

I won't include it in my PR.