Do not include function type conversions in user-defined conversions#56416

Merged

cston merged 6 commits intodotnet:mainfrom

Sep 24, 2021

Contributor

cston commented Sep 15, 2021 •

edited by jcouv

Loading

Removes "function type conversions" from the set of "standard conversions" so that function type conversions (from the inferred type of a lambda expression or method group to Delegate, Expression or a base type) are not considered for user-defined conversions.

Fixes #56407

Relates to test plan #52192

ghost added the Area-Compilers label

cston force-pushed the 56407 branch from 930d756 to cd2a10c Compare

September 15, 2021 18:18

cston commented

View reviewed changes

src/Compilers/CSharp/Portable/Binder/Semantics/Conversions/UserDefinedImplicitConversions.cs Outdated

                               case ConversionKind.DefaultLiteral:
+                              // Added for C# 10.
+                              case ConversionKind.FunctionType:

Contributor Author

cston Sep 15, 2021

case ConversionKind.FunctionType:

Follow up with language design: should a function type conversion be considered an "encompassing" implicit conversion?

cston force-pushed the 56407 branch 3 times, most recently from c9825eb to 958c08f Compare

September 17, 2021 21:52

cston changed the title ~~Allow user-defined conversions from lambda expressions or method groups to Delegate, Expression, and base types~~ Handle user-defined conversions from lambda expressions or method groups to Delegate, Expression, and base types


          Handle user-defined conversions from lambda expressions or method gro…

df95d51

…ups to Delegate, Expression, and base types

cston force-pushed the 56407 branch 2 times, most recently from 68be73b to f8c5df3 Compare

September 19, 2021 04:22


          Remove function type conversions from standard conversions

6539d9a

cston force-pushed the 56407 branch from f8c5df3 to 6539d9a Compare

September 19, 2021 04:44

cston marked this pull request as ready for review

September 19, 2021 05:29

cston requested a review from a team as a code owner

September 19, 2021 05:29

cston changed the title ~~Handle user-defined conversions from lambda expressions or method groups to Delegate, Expression, and base types~~ Do not include function type conversions in user-defined conversions


          Extract local

0b5884c

cston force-pushed the 56407 branch from 0df967e to 0b5884c Compare

September 19, 2021 16:34

cston added the Feature - Lambda Improvements label

cston added this to the 17.0 milestone

AlekseyTs reviewed

View reviewed changes

src/Compilers/CSharp/Portable/Binder/Semantics/Conversions/ConversionsBase.cs Outdated

                           }
+                          else if (sourceExpression.GetFunctionType() is { })
+                          {
+                              if (IsValidFunctionTypeConversionTarget(destination, ref useSiteInfo))

Contributor

AlekseyTs Sep 21, 2021 •

edited

Loading

if (IsValidFunctionTypeConversionTarget(destination, ref useSiteInfo))

Should we also check conversions between two Function Types? #Closed

Contributor Author

cston Sep 22, 2021

The only code paths that should have to handle conversions between two function types are BestTypeInferrer and MethodTypeInferrer in cases where "bounds" are merged. Those code paths use ClassifyImplicitConversionFromTypeOrImplicitFunctionTypeConversion().

Contributor Author

cston Sep 22, 2021

Added handling of conversions between function types.

AlekseyTs reviewed

View reviewed changes

src/Compilers/CSharp/Portable/Binder/Semantics/Conversions/ConversionsBase.cs

+                          }
+                          Debug.Assert(false);
+                          return Conversion.NoConversion;

Contributor

AlekseyTs Sep 21, 2021 •

edited

Loading

return Conversion.NoConversion

It is not clear why conversions to System.MulticastDelegate and the like are not checked. #Closed

Contributor Author

cston Sep 22, 2021

The only callers of this method are BestTypeInferrer and MethodTypeInferrer in cases where "bounds" are merged. In those cases, there are either two function types or two types that are not function types, so conversions from function type to non-function type do not need to be considered.

AlekseyTs reviewed

View reviewed changes

src/Compilers/CSharp/Portable/Binder/Semantics/Conversions/ConversionsBase.cs Outdated

+                          return conversion;
+                      }
+                      private Conversion ClassifyStandardImplicitConversionInternal(TypeSymbol source, TypeSymbol destination, ref CompoundUseSiteInfo<AssemblySymbol> useSiteInfo)

Contributor

AlekseyTs Sep 21, 2021 •

edited

Loading

ClassifyStandardImplicitConversionInternal

It looks like there is only one call site. Consider making this a local function. #Closed

AlekseyTs reviewed

View reviewed changes

src/Compilers/CSharp/Portable/Binder/Semantics/Conversions/ConversionsBase.cs Outdated

                       {
                           Debug.Assert((object)source != null);
                           Debug.Assert((object)destination != null);
+                          Debug.Assert(source is not FunctionTypeSymbol);

Contributor

AlekseyTs Sep 21, 2021 •

edited

Loading

Debug.Assert(source is not FunctionTypeSymbol);

It feels like it would be better to handle this input rather than asserting that it doesn't occur. #Closed

AlekseyTs reviewed

View reviewed changes

src/Compilers/CSharp/Portable/Binder/Semantics/Conversions/ConversionsBase.cs Outdated

+                          {
+                              if (IsValidFunctionTypeConversionTarget(destination, ref useSiteInfo))
+                              {
+                                  return Conversion.FunctionType;

Contributor

AlekseyTs Sep 21, 2021 •

edited

Loading

return Conversion.FunctionType;

Is this an implicit conversion? It feels like the name of the function implies that implicit conversions should be ignored. #Closed

AlekseyTs reviewed

View reviewed changes

src/Compilers/CSharp/Portable/Binder/UsingStatementBinder.cs Outdated

                               return fromExpression ?
-                                  originalBinder.Conversions.ClassifyImplicitConversionFromExpression(expressionOpt, targetInterface, ref useSiteInfo) :
-                                  originalBinder.Conversions.ClassifyImplicitConversionFromType(declarationTypeOpt, targetInterface, ref useSiteInfo);
+                                  originalBinder.Conversions.ClassifyImplicitConversionFromExpression(expressionOpt!, targetInterface, ref useSiteInfo) :

Contributor

AlekseyTs Sep 21, 2021 •

edited

Loading

!

Do we usually prefer asserts to suppressions? #Closed

Contributor

AlekseyTs commented Sep 21, 2021

Done with review pass (commit 3)

cston added 3 commits

September 22, 2021 14:20


          PR feedback

64c63cf


          PR feedback

a37e5f9


          Remove implicit conversion case

7b745c1

cston requested a review from a team

September 22, 2021 23:39

AlekseyTs approved these changes

View reviewed changes

Contributor

AlekseyTs left a comment

LGTM (commit 6)

cston requested a review from a team

September 24, 2021 15:30

333fred approved these changes

View reviewed changes

Member

333fred left a comment

LGTM (commit 6)

cston merged commit 3913f35 into dotnet:main

ghost modified the milestones: 17.0, Next

cston deleted the 56407 branch

September 24, 2021 23:24

Cosifne modified the milestones: Next, 17.0.P5

niemyjski mentioned this pull request

Dotnet post exceptionless/Website#39

Open

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area-Compilers Feature - Lambda Improvements