Run the simplifier over the expression generation tests to fix issue 26586#28831
Run the simplifier over the expression generation tests to fix issue 26586#28831jinujoseph merged 1 commit intodotnet:masterfrom
Conversation
|
Hey @Paxxi , could you update your title to something a bit more descriptive? Thanks! |
There was a problem hiding this comment.
Feels like the nodeCreator is superfluous. Just pass the SyntaxGenerator and the generated expression to put in the local-decl-statement.
There was a problem hiding this comment.
my preference is that TestWithSimplifier would also run the normal test. So this should actually take 4 strings. cs, csSimple, vb, vbSimple.
After all, the point of this file is to test the generator not the simplifier, so we want to ensure we're always at least testing the generator output.
There was a problem hiding this comment.
Then it doesn't make so much sense to have two different test methods. I'll merge it all into the Test method. I initially separated them because some tests appeared to not result in valid syntax when used as a local declaration. Can pass null to csSimple/vbSimple if that's the case then.
There was a problem hiding this comment.
comment above needs to be updated to explain why this is necessary.
I think the fix is likely correct. presumably this is to deal with the |
Nope. That's the correct way to do things. The Simplifier needs real code to work on. Something like "(1) + (1)" is not actually legal C#, so there's no way to properly parse that, let alone doing any semantic analysis of it to determine if can be simplified. |
|
Merged the |
There was a problem hiding this comment.
might be a good idea to assert that at least one of those 4 tests is not null. We've def had cases in the past where you could call a test method with not enough info and always pass :D
There was a problem hiding this comment.
Good idea, I'll add that :)
In addition to testing the expressions as generated, with parenthesis in place, also run them through the simplifier. New Test method helper takes two new parameters, csSimple and vbSimple. When set to null the simplifier step is skipped.
|
@dotnet/roslyn-ide @jinujoseph @jcouv can this test-only change be merged in? thanks! |
Adds a new test helper method that runs the expressions
through the simplifier to remove parenthesis.
New test helper wraps the expression under test in some boilerplate to make sure the simplifier works properly. Without the boilerplate the simplifier never did any work. Did I miss something maybe?
Included also is a fix for one of the
TestMemberAccessOffOfElementAccesstest where the VB simplifier removed parenthesis when it shouldn't. I'm not sure the fix is correct but I included it here to allow for discussion and to get some input on it.I'm not sure about the template and could use some help filling it out if it's required :)
Ask Mode template not completed
Customer scenario
What does the customer do to get into this situation, and why do we think this
is common enough to address for this release. (Granted, sometimes this will be
obvious "Open project, VS crashes" but in general, I need to understand how
common a scenario is)
Bugs this fixes
#26586
Workarounds, if any
Also, why we think they are insufficient for RC vs. RC2, RC3, or RTW
Risk
This is generally a measure our how central the affected code is to adjacent
scenarios and thus how likely your fix is to destabilize a broader area of code
Performance impact
PR only touches test code so low or no perf impact.
Is this a regression from a previous update?
No
Root cause analysis
How did we miss it? What tests are we adding to guard against it in the future?
How was the bug found?
(E.g. customer reported it vs. ad hoc testing)
Test documentation updated?
If this is a new non-compiler feature or a significant improvement to an existing feature, update https://github.com/dotnet/roslyn/wiki/Manual-Testing once you know which release it is targeting.