Move features over to new "simplifier + var" system. by CyrusNajmabadi · Pull Request #26237 · dotnet/roslyn

CyrusNajmabadi · 2018-04-18T19:20:52Z

No description provided.

CyrusNajmabadi · 2018-04-18T22:51:16Z

Tagging @DustinCampbell as i had to make a couple of interesting behavioral choices. I've tagged you where i made them so you can weigh in if the change is ok.

CyrusNajmabadi · 2018-04-18T22:53:46Z

            ImmutableArray.Create<AbstractReducer>(
-                new CSharpCastReducer(),
                new CSharpNameReducer(),
+                new CSharpCastReducer(),


@DustinCampbell This was the interesting starting point. One of our features (convert foreach to for) generates code like so:

IList<int> list = (IList<int>)collection;

Previous behavior was that it wanted this to change into:

var list = (IList<int>)collection;

However, after my change it converted to:

IList<int> list = collection;

This was due to the 'cast' reducer running before the name reducer. IMO, if someone expresses a 'var' preference, i think that should take preference, so i changed to have name-reduction happen before cast-reduction. However, this had observable downstream impacts in a few tests that i've called out.

Take a look at the changed tests and let me know waht you think. Should name reduction come before cast reduction, or vice versa?

I think this is fine.

There was some reason why we reduced casts twice here, but I don't recall what it was and it doesn't appear to have been problematic for any existing tests. Regardless, it always felt like a hack.

I kind of wish the generic method argument simplification was implemented as a separate reducer rather than part of the name reducer so we could just move it below the cast reducer, but I think it's fine for now.

CyrusNajmabadi · 2018-04-18T22:55:22Z

    {
        byte z = 0;
-        Goo<byte, byte>({|Rename:NewMethod|}(), y => { return 0; }, z, z);
+        Goo({|Rename:NewMethod|}(), y => { return (byte)0; }, z, z);


@DustinCampbell see https://github.com/dotnet/roslyn/pull/26237/files#r182592374

Now, we attempt to simplify the name first rather than the cast first, so we get this behavior. IMO, i think the new code is preferable to the old. Using inference for generic methods is very preferred (especially as you might end up having to name a lot of type parameters), so i think we should attempt to preserve inference first, even if it means we have to keep some casts.

Yes, this is definitely better.

CyrusNajmabadi · 2018-04-18T22:56:09Z

    static void M()
    {
-        IComparable<long> c = Goo<long>(1, 1);
+        IComparable<long> c = Goo(1, (long)1);


@DustinCampbell see https://github.com/dotnet/roslyn/pull/26237/files#r182592374

This is the same as the above test. Humorously, this test is called "InsertCastToKeepGenericMethodInference". So it sounds like we did actually intend for their to be a cast, rather than keeping the type parameters.

Yeah, simplification of generic method arguments was added to the system later. I agree that the new version is preferred (at least to my eye). Maybe someday, we'll add simplification to type characters (as an option of course!).

IComparable<long> c = Goo(1, 1L);

😄

CyrusNajmabadi · 2018-04-18T22:56:28Z

        byte z = 0;
        Func<byte, byte> {|Rename:p|} = x => 0;
-        Goo<byte, byte>(p, y => 0, z, z);
+        Goo(p, y => (byte)0, z, z);


@DustinCampbell see https://github.com/dotnet/roslyn/pull/26237/files#r182592374

This is the same as the above test. Humorously, this test is called "InsertNeededCast2". So it sounds like we did actually intend for their to be a cast, rather than keeping the type parameters.

Yup. It was originally a Nikov case. I like the change here.

CyrusNajmabadi · 2018-04-18T22:57:04Z

+    public Enumerator GetEnumerator() { return default; }

-  public struct Enumerator { public object Current { get; } }
+    public struct Enumerator { public object Current { get; } public bool MoveNext() => true; }


needed to make this change, otherwise the compiler felt that the type didn't properly match the 'foreach' pattern, and didn't want to consider hte types the same for reduction.

CyrusNajmabadi · 2018-04-18T22:57:49Z

+                ? foreachInfo.ExplicitCastInterface
+                : model.GetTypeInfo(foreachCollectionExpression).Type ?? model.Compilation.GetSpecialType(SpecialType.System_Object);
+
+            var collectionStatementType = typeSymbol.GenerateTypeSyntax();


these are the places where i updated our features to now just use 'GenerateTypeSyntax()' confident that the simplifier would now use 'var' if htat's what the user wanted.

Awesome. It's definitely simpler.

CyrusNajmabadi · 2018-04-19T01:29:51Z

Tagging @dotnet/roslyn-ide @jcouv @jmarolf as well.

CyrusNajmabadi · 2018-04-19T23:28:06Z

Tagging @dotnet/roslyn-ide @jcouv @heejaechang

CyrusNajmabadi · 2018-04-19T23:28:26Z

Tagging @ivanbasov this will likely impact your foreach/linq work.

heejaechang · 2018-04-19T23:35:26Z

nice! we should have other code styles as well such as having "this." or not and etc.

CyrusNajmabadi · 2018-04-19T23:47:08Z

nice! we should have other code styles as well such as having "this." or not and etc.

Indeed, IMO, all the codestyle stuff should be enforced on nodes marked with the Simplifier Annotatoin when passed through the simplifier.

DustinCampbell · 2018-04-20T14:13:15Z

Indeed, IMO, all the codestyle stuff should be enforced on nodes marked with the Simplifier Annotatoin when passed through the simplifier.

This was always the original intent.

DustinCampbell

I'm definitely in favor of this change. The changes to the unit tests represent simpler, yet still correct, code which was always the intent of the feature. Thanks @CyrusNajmabadi!

DustinCampbell · 2018-04-20T14:15:38Z

    static void M()
    {
-        IComparable<long> c = Goo<long>(1, 1);
+        IComparable<long> c = Goo(1, (long)1);


Yeah, simplification of generic method arguments was added to the system later. I agree that the new version is preferred (at least to my eye). Maybe someday, we'll add simplification to type characters (as an option of course!).

IComparable<long> c = Goo(1, 1L);

😄

DustinCampbell · 2018-04-20T14:15:59Z

    {
        byte z = 0;
-        Goo<byte, byte>({|Rename:NewMethod|}(), y => { return 0; }, z, z);
+        Goo({|Rename:NewMethod|}(), y => { return (byte)0; }, z, z);


Yes, this is definitely better.

DustinCampbell · 2018-04-20T14:17:05Z

        byte z = 0;
        Func<byte, byte> {|Rename:p|} = x => 0;
-        Goo<byte, byte>(p, y => 0, z, z);
+        Goo(p, y => (byte)0, z, z);


Yup. It was originally a Nikov case. I like the change here.

DustinCampbell · 2018-04-20T14:17:26Z

+    public Enumerator GetEnumerator() { return default; }

-  public struct Enumerator { public object Current { get; } }
+    public struct Enumerator { public object Current { get; } public bool MoveNext() => true; }


DustinCampbell · 2018-04-20T14:18:04Z

+                ? foreachInfo.ExplicitCastInterface
+                : model.GetTypeInfo(foreachCollectionExpression).Type ?? model.Compilation.GetSpecialType(SpecialType.System_Object);
+
+            var collectionStatementType = typeSymbol.GenerateTypeSyntax();


Awesome. It's definitely simpler.

DustinCampbell · 2018-04-20T14:21:48Z

            ImmutableArray.Create<AbstractReducer>(
-                new CSharpCastReducer(),
                new CSharpNameReducer(),
+                new CSharpCastReducer(),


I think this is fine.

There was some reason why we reduced casts twice here, but I don't recall what it was and it doesn't appear to have been problematic for any existing tests. Regardless, it always felt like a hack.

I kind of wish the generic method argument simplification was implemented as a separate reducer rather than part of the name reducer so we could just move it below the cast reducer, but I think it's fine for now.

CyrusNajmabadi · 2018-04-20T18:15:28Z

Thanks @DustinCampbell I'll see if i can start pushing more and more stuff to the simplifier.

CyrusNajmabadi · 2018-04-20T18:15:44Z

@heejaechang @jmarolf @DustinCampbell can one of you merge in?

CyrusNajmabadi · 2018-04-20T18:21:02Z

Thanks!

CyrusNajmabadi added 30 commits March 31, 2018 13:52

Initial UseConditional work.

514c920

Initial UseConditional work.

2a38b93

Working replacement.

7f82f3a

Extract common code.

96cd0ab

Make 'var' work properly in the simplifier service.

1561214

Move types to their own files.

78a5872

Provide a way to create types that are not simplified to 'var'.

ec650b9

Update test options.

05b6fb0

Update test.

11371db

Move CSharp code style code down to the Workspace layer.

919ab7a

Rename config value.

3f294cd

Add ui options.

40437e7

Add VB ui options.

3a6ce03

Add VB support

f5e476d

Strip trivia

4076b69

Initial stubs for 'use conditional for returns'

bfd91b2

Add VB impl.

5009d32

Only support processing of real trees.

6cd6ed3

Actually assign variable.

0a8219c

Don't assume a single variable.

7ab2623

Share more code.

186cd32

Fix shared code.

6ab3e4e

Add check.

d1ae6a4

Simplify code.

242b8eb

Only the analyzer needs to check the severity.

f81df9f

Generalize loc strings.

f56a680

remove static.

30c80a8

Remove unneeded methods.

23f8419

Do cheap checks first.

eda264e

Handle more cases.

0945c62

Remove helper. The simplifier will do the right thing now.

ade1dc6

CyrusNajmabadi requested a review from a team as a code owner April 18, 2018 19:20

CyrusNajmabadi added 2 commits April 18, 2018 14:52

try to use var first, before removing casts.

940af3a

Update tests.

1897da0

CyrusNajmabadi changed the title ~~WIP: validate that features can just depend on the simplifier to do the right thing.~~ Falidate that features can just depend on the simplifier to do the right thing wrt var. Apr 18, 2018

CyrusNajmabadi commented Apr 18, 2018

View reviewed changes

CyrusNajmabadi changed the title ~~Falidate that features can just depend on the simplifier to do the right thing wrt var.~~ Validate that features can just depend on the simplifier to do the right thing wrt var. Apr 19, 2018

Merge remote-tracking branch 'upstream/master' into moveVarDown2

33512c0

jaredpar added the Area-IDE label Apr 19, 2018

Merge branch 'master' into moveVarDown2

4a71988

CyrusNajmabadi changed the title ~~Validate that features can just depend on the simplifier to do the right thing wrt var.~~ Move features over to new "simplifier + var" system. Apr 19, 2018

heejaechang approved these changes Apr 19, 2018

View reviewed changes

DustinCampbell approved these changes Apr 20, 2018

View reviewed changes

heejaechang merged commit 04d57fd into dotnet:master Apr 20, 2018

CyrusNajmabadi deleted the moveVarDown2 branch September 17, 2022 04:28

Conversation

CyrusNajmabadi commented Apr 18, 2018

Uh oh!

CyrusNajmabadi commented Apr 18, 2018

Uh oh!

CyrusNajmabadi Apr 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CyrusNajmabadi commented Apr 19, 2018

Uh oh!

CyrusNajmabadi commented Apr 19, 2018

Uh oh!

CyrusNajmabadi commented Apr 19, 2018

Uh oh!

heejaechang commented Apr 19, 2018

Uh oh!

CyrusNajmabadi commented Apr 19, 2018

Uh oh!

DustinCampbell commented Apr 20, 2018

Uh oh!

DustinCampbell left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CyrusNajmabadi commented Apr 20, 2018

Uh oh!

CyrusNajmabadi commented Apr 20, 2018

Uh oh!

CyrusNajmabadi commented Apr 20, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

CyrusNajmabadi Apr 18, 2018 •

edited

Loading