use latest language version for Miscellaneous Files workspace by JieCarolHu · Pull Request #25802 · dotnet/roslyn

JieCarolHu · 2018-03-29T01:43:05Z

use latest language version for Miscellaneous Files workspace

Customer scenario

Diff window uses the default language version instead of the project language version. So when a code file contains a new feature which does not exist in the default language version, the diff window shows error which is confusing to customers.

Bugs this fixes

https://devdiv.visualstudio.com/DevDiv/_workitems/edit/575761

Workarounds, if any

No

Risk

Low, only uses the latest language version instead of the default language version for ParserOptions when opening a historical version of code file

Performance impact

Low, no extra allocations/no complexity changes

Is this a regression from a previous update?

Not sure

Root cause analysis

NA

How was the bug found?

Customer reported on vs feedback

Test documentation updated?

No

sharwell · 2018-03-29T13:23:42Z

... no extra allocations ...

This is not correct. A previously non-allocating path is now always-allocating here:

https://github.com/dotnet/roslyn/blob/abb91c56bf7a133241062aedb7460fe303f0460b/src/Workspaces/CSharp/Portable/LanguageServices/CSharpSyntaxTreeFactoryService.cs#L31

sharwell · 2018-03-29T13:25:38Z

❗️ I would prefer to not change the behavior of GetDefaultParseOptions() (see comment in MiscellaneousFilesWorkspace)

sharwell · 2018-03-29T13:27:22Z

❗️ I would prefer to add the following method to ParseOptions:

internal abstract ParseOptions WithLatestLanguageVersion();

You could then use the following:

var parseOptionsOpt = languageServices.GetService<ISyntaxTreeFactoryService>()?.GetDefaultParseOptions() .WithLatestLanguageVersion();

jcouv · 2018-03-29T22:04:29Z

Tagging @jaredpar @gafter for public API change.

It feels like this new public API is unnecessary.
The consumer can do the job with existing language-specific APIs, so an abstract (language-agnostic) API could be added in IDE layer if you need.

jcouv · 2018-03-29T22:06:39Z

This is wrong (losing other information inside the parse options you start with). See WithFeatures above for contrast.
Assuming we end adding such a compiler API, it needs corresponding API-level compiler tests.

jcouv

Done with review pass (iteration 4).
Let's do this without compiler change.

sharwell · 2018-03-29T22:31:35Z

@dotnet/roslyn-compiler I intended for this to be internal. Please let us know how to define such a method for use in the VS layer without a new public API.

Why not implement this API in one of the language services? (ISyntaxTreeFactoryService seems reasonable place)

If that doesn't work for some reason, you can try making this internal, but I'm not sure there is InternalsVisibleTo from the compiler to this part of the IDE code.
If there is no IVT, there is probably a reason why it's not there, and so I would suspect it shouldn't be added.

Originally, we couldn't even have IVT between compiler and IDE for legal reasons. Today, we simply don't want it because the compiler API should be sufficient for consumer's needs.

If there is concern about a new API why not just make it an extension method? That doesn't need any change in the compiler.

Added to the ISyntaxTreeFactoryService.
I am not sure how to add the override extension method for ParseOptions, VisualBasicParseOptions and CSharpParseOptions, so ISyntaxTreeFactoryService looks like a good place

…actoryService

jcouv · 2018-04-04T19:34:09Z

There is no longer any compiler change in this PR, so I removed the Compiler label. This can be reviewed by IDE team. Thanks

jasonmalinowski · 2018-04-04T19:40:57Z

    [ExportLanguageServiceFactory(typeof(ISyntaxTreeFactoryService), LanguageNames.CSharp), Shared]
    internal partial class CSharpSyntaxTreeFactoryServiceFactory : ILanguageServiceFactory
    {
+        private static CSharpParseOptions _parseOptionWithLatestLanguageVersion = CSharpParseOptions.Default.WithLanguageVersion(LanguageVersion.Latest);


jasonmalinowski · 2018-04-04T19:42:14Z

            var compilationOptionsOpt = languageServices.GetService<ICompilationFactoryService>()?.GetDefaultCompilationOptions();
-            var parseOptionsOpt = languageServices.GetService<ISyntaxTreeFactoryService>()?.GetDefaultParseOptions();
+
+            // We only have one code file without project file here, so we cannot find out language version of the project  


Comment seems out of place since that's the fact of life for this entire type. 😄

updated the comment

jasonmalinowski · 2018-04-04T19:45:53Z

@JieCarolHu Have we tested TypeScript and F# with this? I'm assuming they don't have an implementation of the ISyntaxTreeFactoryService (I don't know how they could...) but if they do this is a breaking change.

jaredpar · 2018-04-04T20:15:09Z


+        Private Shared ReadOnly _parseOptionsWithLatestLanguageVersion As VisualBasicParseOptions = VisualBasicParseOptions.Default.WithLanguageVersion(LanguageVersion.Latest)
+
+


Nit: double blank line

JieCarolHu · 2018-04-04T20:22:51Z

@jasonmalinowski tested on typescript and F#, none of them creates project for miscellaneous files. So they are not affected by this change

jasonmalinowski · 2018-04-04T20:24:36Z

@JieCarolHu Do they have implementations of this interface, even if they don't use Miscellaneous Files? Adding new members to interfaces is breaking unless we are careful.

JieCarolHu · 2018-04-04T21:16:57Z

@jasonmalinowski Actually I was searching for the wrong interface. Just searched for "ISyntaxTreeFactoryService" again and they did not implement this interface

JieCarolHu · 2018-04-04T22:22:24Z

@jaredpar @sharwell Could you guys please finish review? Thanks

jaredpar · 2018-04-04T22:24:14Z

@JieCarolHu sorry didn't realize you were waiting for my sign off since you removed the compiler change. The changes looked good to me.

jinujoseph · 2018-04-04T23:32:38Z

@Pilchie for ask mode approval

JieCarolHu · 2018-04-04T23:35:41Z

@Pilchie for ask mode approval, thanks!

Pilchie · 2018-04-05T00:55:56Z

Approved for 15.7 Preview 4.

JieCarolHu requested a review from a team as a code owner March 29, 2018 01:43

sharwell suggested changes Mar 29, 2018

View reviewed changes

JieCarolHu requested a review from a team as a code owner March 29, 2018 21:44

jcouv reviewed Mar 29, 2018

View reviewed changes

jcouv added Area-Compilers Area-IDE labels Mar 29, 2018

jcouv added this to the 15.7 milestone Mar 29, 2018

sharwell suggested changes Mar 29, 2018

View reviewed changes

Add GetDefaultParseOptionsWithLatestLanguageVersion() to ISyntaxTreeF…

d54cbc4

…actoryService

JieCarolHu force-pushed the vs575761 branch from 67cc071 to d54cbc4 Compare March 31, 2018 00:44

use Default.WithLanguageVersion()

589ec3c

JieCarolHu requested review from jaredpar, jasonmalinowski, jcouv and sharwell March 31, 2018 00:58

jcouv removed the Area-Compilers label Mar 31, 2018

jasonmalinowski reviewed Apr 4, 2018

View reviewed changes

readonly, updated comments

6c35eed

jaredpar reviewed Apr 4, 2018

View reviewed changes

whitespace

be4a1e5

jasonmalinowski approved these changes Apr 4, 2018

View reviewed changes

jaredpar approved these changes Apr 4, 2018

View reviewed changes

sharwell approved these changes Apr 4, 2018

View reviewed changes

Pilchie approved these changes Apr 5, 2018

View reviewed changes

Pilchie added the Approved to merge label Apr 5, 2018

JieCarolHu merged commit 902e902 into dotnet:dev15.7.x Apr 5, 2018

JieCarolHu deleted the vs575761 branch April 5, 2018 19:26


		Private Shared ReadOnly _parseOptionsWithLatestLanguageVersion As VisualBasicParseOptions = VisualBasicParseOptions.Default.WithLanguageVersion(LanguageVersion.Latest)

Conversation

JieCarolHu commented Mar 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Customer scenario

Bugs this fixes

Workarounds, if any

Risk

Performance impact

Is this a regression from a previous update?

Root cause analysis

How was the bug found?

Test documentation updated?

Uh oh!

sharwell commented Mar 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sharwell Mar 29, 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!

jcouv Mar 29, 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!

jcouv 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!

jcouv commented Apr 4, 2018

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!

jasonmalinowski commented Apr 4, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JieCarolHu commented Apr 4, 2018

Uh oh!

jasonmalinowski commented Apr 4, 2018

Uh oh!

JieCarolHu commented Apr 4, 2018

Uh oh!

JieCarolHu commented Apr 4, 2018

Uh oh!

jaredpar commented Apr 4, 2018

Uh oh!

jinujoseph commented Apr 4, 2018

Uh oh!

JieCarolHu commented Apr 4, 2018

Uh oh!

Pilchie commented Apr 5, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

JieCarolHu commented Mar 29, 2018 •

edited

Loading

sharwell commented Mar 29, 2018 •

edited

Loading

sharwell Mar 29, 2018 •

edited

Loading

jcouv Mar 29, 2018 •

edited

Loading