Use 7.2 language features in Roslyn by VSadov · Pull Request #26010 · dotnet/roslyn

VSadov · 2018-04-06T23:57:27Z

Trying to use the new language features in the Roslyn codebase.

VSadov · 2018-04-07T00:28:37Z

@dotnet/roslyn-compiler - FYI. Trying to use new language features in Roslyn.

VSadov · 2018-04-07T00:29:59Z

Ref reassignments work very nicely in the AVL tree implementation of SmallDictionary. 👍

VSadov · 2018-04-07T00:39:47Z

readonly structs and in parameters work fine too.

Another nice part is that the changes are not disrupting existing code and can be done incrementally.
That was one of the design goals.

Neme12 · 2018-04-07T00:53:52Z

I like this... this is how I usually implement things in C++, it simplifies the code a lot and also removes a special case for inserting the root node.

also removes a special case for inserting the root node

would that be possible here by restructuring the loop and moving the null check?

sharwell · 2018-04-07T02:30:20Z

❓ Why explicit? At this point it behaves like passing by value which seems to be less noisy. #WontFix

It was explicit ref originally. I assumed that ref was used for performance which makes it important that we continue passing the token by direct reference. That is where you use in at the call site - to make sure there is no copying.

Basically - I used the "ref for performance reasons" pattern as an indication of perf-sensitive codepaths and used explicit in there.
I think it makes sense to prevent unexpected perf regressions or at least as a self-documenting way to say that avoiding copies is very important. #WontFix

sharwell · 2018-04-07T02:32:04Z

❓ What prevents this in C# 7.2? If it's allowed, it would be helpful in the review if the C# 7.2 features could be brought in separately from the C# 7.3 features. #Resolved

I did not realize that 7.2 is no longer barred. I will separate 7.3 into a separate PR, so that this could actually get merged.

In reply to: 179906078 [](ancestors = 179906078)

Note that I still need to update the toolset compiler packages to 2.7.0 which matches Visual Studio 2017 version 15.6
There was a number of important fixes and it seems the changed code is relying on some of them.

In reply to: 180535550 [](ancestors = 180535550,179906078)

jaredpar

VSadov · 2018-04-11T01:00:12Z

@dotnet/roslyn-compiler - ok, since we want to merge this, I need one more review. Thanks.

VSadov · 2018-04-11T01:05:03Z

The 7.3 part is now a separate PR - #26092

That is not going to be merged. Not until after 7.3 is officially released.

sharwell · 2018-04-11T17:33:10Z

        public Enumerator GetEnumerator()
        {
-            return new Enumerator(this);
+            return new Enumerator(in this);


❓ Why make this change? The current structure appears to only have one field, which is SyntaxNode (a reference type) and therefore be cheapest to copy by value. #Resolved

Will take a look. I may have mixed it up with other cases. There would not be much point to pass a single reference by in. #Resolved

jasonmalinowski · 2018-04-11T19:49:02Z

 Microsoft.CodeAnalysis.SyntaxTokenList.Reversed
 Microsoft.CodeAnalysis.SyntaxTokenList.Reversed.Enumerator
 Microsoft.CodeAnalysis.SyntaxTokenList.Reversed.Enumerator.Current.get -> Microsoft.CodeAnalysis.SyntaxToken
-Microsoft.CodeAnalysis.SyntaxTokenList.Reversed.Enumerator.Enumerator(ref Microsoft.CodeAnalysis.SyntaxTokenList list) -> void


Breaking change? #ByDesign

Yes, but only with specific compiler versions (or newer): dotnet/csharplang#1308 #ByDesign

(which might be even worse than a normal breaking change 😄) #WontFix

Technically yes. But I am certain that it was public by mistake. Noone should be instantiating these enumerators except via GetEnumerator call. No other struct enumerator have a public constructor. Especially those that take byre parameters.

We can discuss this further but while most compat breaks are bad, not all are. I think this public exposure should be undone ASAP. - not just because it stands in the way of changing from ref -> in, I could leave it as ref. I left ref in other public places, for compat reasons.

Here a public ctor just makes no sense.

In reply to: 180876819 [](ancestors = 180876819)

I am simply removing the ctors. It is not related to in vs. ref

In reply to: 180885818 [](ancestors = 180885818)

I have sent a note to Compat Council to decide on this, but I think we should remove these from the public API.

I can keep the ref ctors public (for what use?) and add in ctor with a dummy parameter for the use in GetEnumerator - to avoid defensive copying, but it would be a very counterproductive "fix"

In reply to: 180900004 [](ancestors = 180900004,180876819)

Compat Council is ok with removing the following from the public surface:

Microsoft.CodeAnalysis.SyntaxTokenList.Reversed.Enumerator.Enumerator(ref Microsoft.CodeAnalysis.SyntaxTokenList list)

Microsoft.CodeAnalysis.SyntaxTriviaList.Reversed.Enumerator.Enumerator(ref Microsoft.CodeAnalysis.SyntaxTriviaList list)

In reply to: 180905813 [](ancestors = 180905813,180900004,180876819)

VSadov · 2018-04-12T01:14:07Z

@dotnet/roslyn-compiler - PING

agocke

LGTM

VSadov · 2018-04-12T19:49:30Z

Why tests are getting stuck and not starting?

jaredpar · 2018-04-12T19:53:00Z

@VSadov I sent an email about this in the AM. 😄

VSadov · 2018-04-12T23:54:50Z

@jaredpar I thought the issue was resolved and I may need just cycle through Close/Reopen that typically resolves "stuck" tests issue.

VSadov · 2018-04-12T23:55:04Z

Oh well..

OmarTawfik

Awesome! 👍

jaredpar · 2018-04-13T00:26:25Z

@VSadov looks like the required tests have all passed now.

VSadov · 2018-04-13T16:07:12Z

Thanks!!

jcouv · 2018-04-16T15:53:21Z

@VSadov FYI, it looks like we had missed "readonly" completion during IDE testing. See #19216
Did you notice anything strange while dogfooding the feature?

Borrowed from dotnet/roslyn#26010

VSadov requested review from a team as code owners April 6, 2018 23:57

VSadov changed the title ~~[WIP] Use new features~~ [WIP] Use new language features in Roslyn Apr 7, 2018

Neme12 reviewed Apr 7, 2018

View reviewed changes

sharwell reviewed Apr 7, 2018

View reviewed changes

jcouv added Area-Compilers PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. labels Apr 7, 2018

jaredpar approved these changes Apr 10, 2018

View reviewed changes

VSadov force-pushed the useNewFeatures branch from 2fdc752 to de1c418 Compare April 10, 2018 20:02

VSadov removed the PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. label Apr 10, 2018

VSadov added 7 commits April 10, 2018 16:32

move compiler package to 2.7 (matches Visual Studio 2017 version 15.6)

5529bed

use refs in TextKeyedCache

906dcdd

Make Blender and BlendedNode readonly structs. These structs are huge.

0bdbab7

use readonly in syntax core

97e58e3

text primitives are readonly structs

66ddc43

for VB

fcf9d83

Some more use of readonly

02db234

VSadov force-pushed the useNewFeatures branch from 794c8e8 to 02db234 Compare April 10, 2018 23:32

VSadov changed the title ~~[WIP] Use new language features in Roslyn~~ Use 7.2 language features in Roslyn Apr 11, 2018

sharwell reviewed Apr 11, 2018

View reviewed changes

jasonmalinowski reviewed Apr 11, 2018

View reviewed changes

VSadov added 2 commits April 11, 2018 15:54

PR feedback

62414bf

Fix build

df4a2f4

agocke approved these changes Apr 12, 2018

View reviewed changes

VSadov closed this Apr 12, 2018

VSadov reopened this Apr 12, 2018

VSadov closed this Apr 12, 2018

VSadov reopened this Apr 12, 2018

OmarTawfik approved these changes Apr 13, 2018

View reviewed changes

VSadov merged commit d2d03c8 into dotnet:master Apr 13, 2018

VSadov deleted the useNewFeatures branch April 13, 2018 16:07

jcouv mentioned this pull request Apr 16, 2018

Test plan for "ref readonly" and related features (7.2) #19216

Closed

garuma added a commit to KirillOsenkov/XmlParser that referenced this pull request Apr 22, 2018

Some more C# 7.2 perf-related improvements

45ebf67

Borrowed from dotnet/roslyn#26010

Conversation

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

Uh oh!

Uh oh!

VSadov commented Apr 7, 2018

Uh oh!

VSadov commented Apr 7, 2018

Uh oh!

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

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sharwell Apr 7, 2018 • edited by VSadov Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

VSadov Apr 10, 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!

sharwell Apr 7, 2018 • edited by VSadov 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!

jaredpar left a comment

Choose a reason for hiding this comment

Uh oh!

VSadov commented Apr 11, 2018

Uh oh!

VSadov commented Apr 11, 2018

Uh oh!

sharwell Apr 11, 2018 • edited by VSadov Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

VSadov Apr 11, 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!

jasonmalinowski Apr 11, 2018 • edited by VSadov Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tannergooding Apr 11, 2018 • edited by VSadov Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tannergooding Apr 11, 2018 • edited by VSadov 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!

VSadov commented Apr 12, 2018

Uh oh!

agocke left a comment

Choose a reason for hiding this comment

Uh oh!

VSadov commented Apr 12, 2018

Uh oh!

jaredpar commented Apr 12, 2018

Uh oh!

VSadov commented Apr 12, 2018

Uh oh!

VSadov commented Apr 12, 2018

Uh oh!

OmarTawfik left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

VSadov commented Apr 6, 2018 •

edited

Loading

VSadov commented Apr 7, 2018 •

edited

Loading

sharwell Apr 7, 2018 •

edited by VSadov

Loading

VSadov Apr 10, 2018 •

edited

Loading

sharwell Apr 7, 2018 •

edited by VSadov

Loading

sharwell Apr 11, 2018 •

edited by VSadov

Loading

VSadov Apr 11, 2018 •

edited

Loading

jasonmalinowski Apr 11, 2018 •

edited by VSadov

Loading

tannergooding Apr 11, 2018 •

edited by VSadov

Loading

tannergooding Apr 11, 2018 •

edited by VSadov

Loading

OmarTawfik left a comment •

edited

Loading