Skip to content

Stop Simplifier.ReduceAsync removing empty arg lists from delegates#40922

Merged
mavasani merged 15 commits intodotnet:masterfrom
GrahamTheCoder:issue/40442/vb-simplification-arglist
Feb 3, 2020
Merged

Stop Simplifier.ReduceAsync removing empty arg lists from delegates#40922
mavasani merged 15 commits intodotnet:masterfrom
GrahamTheCoder:issue/40442/vb-simplification-arglist

Conversation

@GrahamTheCoder
Copy link
Contributor

@GrahamTheCoder GrahamTheCoder commented Jan 12, 2020

Fixes #40442

  • I suspect the test is in slightly the wrong place, but couldn't find a sensible place.
  • I've aimed to err on the side of caution, leaving the brackets where the symbol is unresolvable except in some cases where it couldn't be anything else valid. If you can point at the spec or code that decides whether something without an arg list will be invoked then I'll happily update to use the precise logic if it differs from what I've done.

Drawbacks:
* Only covers cases where the invocation is nested within an executable statement
* Performs abysmally due to creating a whole speculative semantic model for the vast majority of invocations, and additionally using GetOperation which is quite slow in general

At some point amongst all the code that runs there is presumably a crucial function which has the rules for binding a name-type syntax into an invocation.
Ideally we'd find a way to call just that function here on invocationExpression.Expression
@GrahamTheCoder GrahamTheCoder requested a review from a team as a code owner January 12, 2020 21:43
@GrahamTheCoder GrahamTheCoder changed the title Issue/40442/vb simplification arglist Stop Simplifier.ReduceAsync removing empty arg lists from delegates Jan 12, 2020
@GrahamTheCoder GrahamTheCoder force-pushed the issue/40442/vb-simplification-arglist branch from a0e56e1 to 4685e06 Compare January 12, 2020 21:46
(even without an arg list)

It's possible there are some other cases when VB will happily invoke something without an arg list, but since i can't find the relevant spec or code, I'm erring on the side of caution
@GrahamTheCoder GrahamTheCoder force-pushed the issue/40442/vb-simplification-arglist branch from 4685e06 to 37a230c Compare January 12, 2020 21:47
@CyrusNajmabadi
Copy link
Contributor

I suspect the test is in slightly the wrong place, but couldn't find a sensible place.

This file seems reasonable.

Copy link
Contributor

@CyrusNajmabadi CyrusNajmabadi left a comment

Choose a reason for hiding this comment

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

LGTM if we break the test into a few separate tests.

@CyrusNajmabadi
Copy link
Contributor

From this point on, can you avoid force pushing? It makes it difficult to review things . Thanks!

@CyrusNajmabadi
Copy link
Contributor

Thanks for the PR @GrahamTheCoder !

@jinujoseph jinujoseph added Area-Analyzers Community The pull request was submitted by a contributor who is not a Microsoft employee. labels Jan 13, 2020
Therefore we can't know it's safe to remove parentheses since it could be a delegate
This is to allow "TestInsertCallIfNecessary3()" to continue to pass.
Note that the test result isn't fully simplified since it says:
Call (Sub() Exit Sub)
instead of
Call Sub() Exit Sub
This is outside the scope of this PR
@GrahamTheCoder
Copy link
Contributor Author

I'll try to have a look at the other affected tests in the next day or so. Looks like TestFixedNullExceptionCrash should have been simplified assuming the symbol could be found.

There is no parameterless Console.Write hence it doesn't resolve, and Program doesn't exist in the context at all
Don't think it's worth considering CandidateSymbols since they won't always be correct

Expanding and reducing stuff that doesn't compile sounds generally risky. In a future PR perhaps the expander shouldn't add the parentheses if it can't resolve the symbol.
If it wasn't invokable it wouldn't be valid at this point
@GrahamTheCoder
Copy link
Contributor Author

I'm assuming this is now just in a queue to look at - let me know if there's anything else I can do to help it along

@mavasani mavasani added this to the 16.5.P3 milestone Feb 3, 2020
@mavasani mavasani merged commit 303d2ed into dotnet:master Feb 3, 2020
@GrahamTheCoder GrahamTheCoder deleted the issue/40442/vb-simplification-arglist branch February 3, 2020 19:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area-Analyzers 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.

Simplifier.ReduceAsync changes function evaluation behaviour

4 participants