Add new API Compilation.IsAssignableTo(fromType, toType) by gafter · Pull Request #26719 · dotnet/roslyn

gafter · 2018-05-08T16:00:37Z

We also remove the similar internal method from ISemanticFactsService
Fixes #9461

/cc @CyrusNajmabadi for preliminary review

We also remove the similar internal method from ISemanticFactsService Fixes dotnet#9461

CyrusNajmabadi · 2018-05-08T16:40:17Z

Code change looks fine. But as per the issue discussion, i would want an exhaustive matrix here. i.e. have every type of type on the rows, and every type of type on hte columns, and check them all against each other (with expected positive/negative cases filled in for each intersection).

If that reveals no problems, then this seems fine to me :)

JohanLarsson · 2018-05-08T16:46:25Z

There is an extension method opportunity here.

fromType.IsAssignableTo(toType, compilation)

^ reads natural to me.

If there is a way to dig out the compilation from the type symbol to avoid passing in compilation it would be sweet.

AlekseyTs · 2018-05-08T17:10:30Z

src/Compilers/Core/Portable/Compilation/Compilation.cs

+        /// Returns true if there is an implicit (C#) or widening (VB) conversion from
+        /// <paramref name="fromType"/> to <paramref name="toType"/>.
+        /// </summary>
+        public abstract bool IsAssignableTo(ITypeSymbol fromType, ITypeSymbol toType);


IsAssignableTo [](start = 29, length = 14)

The name sounds very similar to a well known API Type.IsAssignableFrom, but according to the doc comment doesn't quite match the behavior. This could be a source of big confusion for consumers and as a result a source of misuse of the API. #Resolved

Consider renaming to "IsSafeToConvertTo", "SafeConversionExists",or something similar.

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

I like IsAssignableTo, short & clear. #Resolved

Also the fact that in VB a widening conversion is not a requirement for things to be assignable should be taken into consideration when choosing the name of this API #Resolved

@AlekseyTs can you give some examples? it would be super helpful for getting everyone understanding this space better. Thanks! #Resolved

What does that mean though? In particular, what do you define "implicit" as. And i'm with Aleksey, what does this mean in VB? #Resolved

I'm willing to look through the links, but it would be useful to have primary use cases where the feature can describe precisely the sort of question it is trying to ask (in a non-self referential fashion). i.e. we shouldn't say "teh feature is trying to know if it's an implicit conversion". We should say "teh feature is trying to decide if applying the conversion at runtime would succeed or not". #Resolved

For example, VB says this: "Implicit conversions occur without any special syntax". Is it true then that .isWidening is the right impl to answer that question? To me, it looks like no. For example:

If permissive semantics are being used, all widening and narrowing conversions (in other words, all conversions) may occur implicitly.

#Resolved

There are places in C# where non-implicit conversions occur implicitly (e.g. in a foreach loop). I'll make sure the doc comments are clear.

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

Since we now have a team for deciding public CodeAnalysis API changes, I'll submit this question to that group. (and point them to this thread for context)

In reply to: 186923887 [](ancestors = 186923887,186879526)

AlekseyTs · 2018-05-08T17:12:23Z

src/Compilers/CSharp/Portable/Compilation/CSharpCompilation.cs

+        /// Returns true if there is an implicit conversion from
+        /// <paramref name="fromType"/> to <paramref name="toType"/>.
+        /// </summary>
+        public override bool IsAssignableTo(ITypeSymbol fromType, ITypeSymbol toType)


public override bool IsAssignableTo(ITypeSymbol fromType, ITypeSymbol toType) [](start = 8, length = 77)

Consider moving this method next to the ClassifyConversion that is calls. #ByDesign

I'd prefer to keep it in the region that correctly describes its place in the hierarchy.

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

AlekseyTs · 2018-05-08T17:13:21Z

src/Compilers/VisualBasic/Portable/Compilation/VisualBasicCompilation.vb

+        ''' Returns true if there is a widening conversion from
+        ''' <paramref name="fromType"/> to <paramref name="toType"/>.
+        ''' </summary>
+        Public Overrides Function IsAssignableTo(fromType As ITypeSymbol, toType As ITypeSymbol) As Boolean


Public Overrides Function IsAssignableTo(fromType As ITypeSymbol, toType As ITypeSymbol) As Boolean [](start = 8, length = 99)

Same suggestion as for C# #ByDesign

This region properly classifies the method.

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

AlekseyTs · 2018-05-08T17:14:45Z

src/Compilers/Core/CodeAnalysisTest/Resources.Designer.cs

 // <auto-generated>
 //     This code was generated by a tool.
-//     Runtime Version:4.0.30319.34011
+//     Runtime Version:4.0.30319.42000


This file doesn't have any meaningful changes, consider reverting #Resolved

AlekseyTs · 2018-05-08T17:21:39Z

Done with review pass (iteration 1)

CyrusNajmabadi · 2018-05-08T17:24:37Z

@dotnet/analyzer-ioperation @333fred @mavasani If this gets added to the core Compilation API, can we add this to CommonConversion in IOperation as well?

mavasani · 2018-05-08T18:14:43Z

This is a super helpful API - I think we have more than 10 such operation/symbol analyzers in FxCop analyzers package that need C# and VB implementations just to get this functionality. I will file a separate issue to track adding IOperation support for this.

gafter · 2018-05-09T04:05:53Z

This PR is pending review and suggestions by the CodeAnalysis API team, half of which are at Build now. Until they are back this is on hold.

gafter · 2018-05-14T19:14:54Z

I renamed it to IsImplicitConversion but I think HasImplicitConversion would be more grammatically correct.

CyrusNajmabadi · 2018-05-15T18:19:07Z

src/Compilers/Core/Portable/Compilation/Compilation.cs

+        /// either <paramref name="fromType"/> or <paramref name="toType"/> is null, or
+        /// if no such conversion exists.
+        /// </summary>
+        public abstract bool HasImplicitConversion(ITypeSymbol fromType, ITypeSymbol toType);


So, i'm on the fence about this entire PR :) mainly because of how many different ways it seems to be able to determine this sort of thing. i.e. we have:

CommonCompilation.HasImplicitConversion, then we have:
CSharpCompilation/BasicComplation.ClassifyConversion, then we have
IOperation.IConversionOperation/CommonConversion

My main issue is that it feels like this doesn't go far enough. For example, what if i wanted ot ask "Is there a reference conversion between these two types?" Now i'm back where i started.

Please don't shoot me, but i think my preference here would be that instead of having this API we take the following two steps:

Move a ClassifyCommonConversion call down to Compilation (and have it return a CommonConversion).

Add 'IsImplicit' to CommonConversion and document (like here) that it means 'implicit' for C# and 'widening' for VB.

This would seem to solve the motivating scenario, as someone could call compilation.ClassifyCommonConversion(...).IsImplicit, but it would also still expose all the rest of hte information while being consistent with teh types/shapes we use elsewhere for talking about this stuff.

With this, we could still have HasImplicitConversion. But it could just be an extension method on Compilation, that called hte ClassifyCommonConversion helper.

Thoughts, @gafter? This PR feels a bit too "one-off" to me. I think we should go for a more generalized approach that fits in better with the shape of our API elsewhere. #Resolved

I think this is a great suggestion. I will investigate more. #Resolved

+1 #Resolved

Note: one unfortunate issue here is with naming. We can't actually have "ClassifyConversion" on Compilatoin as that would now shadow the extensions we have for the language specific ClassifyConversion extensions. Hence why it would be called ClassifyCommonConversion instead. But i feel like that's not too bad, especially given the different types of results actually returned. #Resolved

CyrusNajmabadi

In terms of exposing the HasImplicitConversoin method, i think this PR is fine. However... i've given feedback that i think overall we could still answer that questoin in a more general and cleaner fashion. Thanks!

…sImplicit

gafter · 2018-05-17T17:18:07Z

@CyrusNajmabadi Please have a look at this revised proposed API. What do you think?

…-9461

333fred · 2018-05-17T19:43:59Z

src/Test/Utilities/Portable/Compilation/CompilationExtensions.cs

 namespace Microsoft.CodeAnalysis.Test.Utilities
 {
-    public static class CompilationExtensions
+    public static class CompilationExtensions2


Rather than 2, can we do TestCompilationExtensions? Suffixing 2 feels wrong. #Resolved

Or CompilationTestExtensions #Resolved

…-9461

gafter · 2018-05-24T00:45:52Z

Review of approved API moved to #27093

Add new API Compilation.IsAssignableTo(fromType, toType)

deccdf8

We also remove the similar internal method from ISemanticFactsService Fixes dotnet#9461

gafter added Concept-API This issue involves adding, removing, clarification, or modification of an API. Area-Compilers Feature Request PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. labels May 8, 2018

gafter added this to the 15.8 milestone May 8, 2018

gafter self-assigned this May 8, 2018

gafter requested review from a team as code owners May 8, 2018 16:00

AlekseyTs reviewed May 8, 2018

View reviewed changes

rename to Compilation.IsImplicitConversion(fromType, toType)

637b5dd

Rename IsImplicitConversion to HasImplicitConversion.

259db3c

gafter mentioned this pull request May 15, 2018

Expose IsAssignableTo from SemanticModel #9461

Closed

CyrusNajmabadi reviewed May 15, 2018

View reviewed changes

CyrusNajmabadi suggested changes May 15, 2018

View reviewed changes

Implement Compilation.ClassifyCommonConversion and CommonConversion.I…

bb5e6ce

…sImplicit

gafter added 2 commits May 17, 2018 10:29

Merge branch 'master' of https://github.com/dotnet/roslyn into master…

14e614a

…-9461

Adjust code post merge.

6601478

333fred reviewed May 17, 2018

View reviewed changes

gafter added 4 commits May 21, 2018 12:28

Rename test helper utility class.

320b1d7

Merge branch 'master' of https://github.com/dotnet/roslyn into master…

b4eed76

…-9461

Revert a previous rename that is no longer needed.

d76e8f8

Merge branch 'master' of https://github.com/dotnet/roslyn into master…

dedff77

…-9461

gafter mentioned this pull request May 24, 2018

Add API Compilation.ClassifyCommonConverstion #27093

Merged

gafter closed this May 24, 2018

mikkelbu mentioned this pull request Oct 30, 2019

NUnit1001: False positive nunit/nunit.analyzers#162

Closed

Conversation

gafter commented May 8, 2018

Uh oh!

CyrusNajmabadi commented May 8, 2018

Uh oh!

JohanLarsson commented May 8, 2018

Uh oh!

AlekseyTs May 8, 2018 • edited by gafter Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs May 8, 2018

Choose a reason for hiding this comment

Uh oh!

JohanLarsson May 8, 2018 • edited by gafter Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs May 8, 2018 • edited by gafter Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CyrusNajmabadi May 8, 2018 • edited by gafter Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CyrusNajmabadi May 8, 2018 • edited by gafter Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CyrusNajmabadi May 8, 2018 • edited by gafter Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CyrusNajmabadi May 8, 2018 • edited by gafter Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gafter May 9, 2018

Choose a reason for hiding this comment

Uh oh!

gafter May 9, 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!

AlekseyTs May 8, 2018 • edited by gafter Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gafter May 9, 2018

Choose a reason for hiding this comment

Uh oh!

AlekseyTs May 8, 2018 • edited by gafter Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gafter May 9, 2018

Choose a reason for hiding this comment

Uh oh!

AlekseyTs May 8, 2018 • edited by gafter Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs commented May 8, 2018

Uh oh!

CyrusNajmabadi commented May 8, 2018

Uh oh!

mavasani commented May 8, 2018

Uh oh!

gafter commented May 9, 2018

Uh oh!

gafter commented May 14, 2018

Uh oh!

CyrusNajmabadi May 15, 2018 • edited by gafter Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gafter May 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

AlekseyTs May 8, 2018 •

edited by gafter

Loading

JohanLarsson May 8, 2018 •

edited by gafter

Loading

AlekseyTs May 8, 2018 •

edited by gafter

Loading

CyrusNajmabadi May 8, 2018 •

edited by gafter

Loading

CyrusNajmabadi May 8, 2018 •

edited by gafter

Loading

CyrusNajmabadi May 8, 2018 •

edited by gafter

Loading

CyrusNajmabadi May 8, 2018 •

edited by gafter

Loading

gafter May 9, 2018 •

edited

Loading

AlekseyTs May 8, 2018 •

edited by gafter

Loading

AlekseyTs May 8, 2018 •

edited by gafter

Loading

AlekseyTs May 8, 2018 •

edited by gafter

Loading

CyrusNajmabadi May 15, 2018 •

edited by gafter

Loading

gafter May 15, 2018 •

edited

Loading

mavasani May 15, 2018 •

edited by gafter

Loading

CyrusNajmabadi May 15, 2018 •

edited by gafter

Loading

333fred May 17, 2018 •

edited by gafter

Loading

333fred May 17, 2018 •

edited by gafter

Loading