Support Delegate and MulticastDelegate as generic type constraints #24443

OmarTawfik · 2018-01-25T06:08:52Z

Enables using System.Delegate and System.MulticastDelegate as constraints for generic parameters.

Example:

class Test<T> where T : System.Delegate {}
class Test<T> where T : System.MulticastDelegate {}

Roslyn Proposal: #158
Corefx Proposal: dotnet/csharplang#86
Champion Issue: dotnet/csharplang#103

@dotnet/roslyn-compiler @dotnet/roslyn-ide @VSadov for review

cston · 2018-01-25T19:15:01Z

src/Compilers/CSharp/Test/Symbol/SymbolDisplay/SymbolDisplayTests.cs

+                SymbolDisplayPartKind.NamespaceName,
+                SymbolDisplayPartKind.Punctuation,
+                SymbolDisplayPartKind.ClassName);
+        }


Consider tests that check the display string for the containing type for each of these cases, using new SymbolDisplayFormat(genericOptions: SymbolDisplayGenericsOptions.IncludeTypeParameters | SymbolDisplayGenericsOptions.IncludeTypeConstraints). #Resolved

cston · 2018-01-25T19:18:21Z

src/Compilers/VisualBasic/Test/Symbol/SymbolsTests/GenericConstraintTests.vb

+        Dim c = new Test(Of string)()         ' reference type
+                            ~~~~~~
+</expected>)
+        End Sub


Consider testing New Test(Of Delegate) and New Test(Of MulticastDelegate) in each case. #Resolved

CyrusNajmabadi · 2018-01-25T19:20:26Z

Sorry if i missed it. But has a reason been given to not use the more C#-keywordy-centric form:

where T : enum or where T : delegate?

It seems like those would make much more sense from a syntactic perspective for users as those are already the existing keywords needed to describe a delegate/enum today. For example, users don't say:

struct S : System.Enum { }

They say: enum S { }

In no other parts of the language have we generally exposed these concepts through System.Enum or System.Delegate/MulticastDelegate. So it seems odd to use the fully qualified names here in constraints when we already have keywords. #ByDesign

cston · 2018-01-25T19:26:28Z

src/Compilers/CSharp/Test/Semantic/Semantics/GenericConstraintsTests.cs

+                // (15,30): error CS0311: The type 'System.Delegate' cannot be used as type parameter 'U' in the generic type or method 'Test<T, U>'. There is no implicit reference conversion from 'System.Delegate' to 'D1'.
+                //         var d = new Test<D1, System.Delegate>();
+                Diagnostic(ErrorCode.ERR_GenericConstraintNotSatisfiedRefType, "System.Delegate").WithArguments("Test<T, U>", "D1", "U", "System.Delegate").WithLocation(15, 30));
+        }


} [](start = 8, length = 1)

Consider testing Test<Delegate, MulticastDelegate>, Test<MulticastDelegate, Delegate>, Test<MulticastDelegate, MulticastDelegate>. #Resolved

cston · 2018-01-25T19:27:11Z

src/Compilers/CSharp/Test/Semantic/Semantics/GenericConstraintsTests.cs

+                // (15,30): error CS0311: The type 'System.MulticastDelegate' cannot be used as type parameter 'U' in the generic type or method 'Test<T, U>'. There is no implicit reference conversion from 'System.MulticastDelegate' to 'D1'.
+                //         var d = new Test<D1, System.MulticastDelegate>();
+                Diagnostic(ErrorCode.ERR_GenericConstraintNotSatisfiedRefType, "System.MulticastDelegate").WithArguments("Test<T, U>", "D1", "U", "System.MulticastDelegate").WithLocation(15, 30));
+        }


} [](start = 8, length = 1)

Consider testing Test<Delegate, Delegate>, Test<Delegate, MulticastDelegate>, Test<MulticastDelegate, Delegate>. #Resolved

cston · 2018-01-25T19:33:15Z

src/Compilers/CSharp/Test/Semantic/Semantics/GenericConstraintsTests.cs

+            CompileAndVerify(code, expectedOutput: @"
+Got 2 and 3
+Got 7 and 9");
+        }


} [](start = 8, length = 1)

Consider testing:

class A1<T> where T : Delegate { } class B1<T> : A1<T> where T : MulticastDelegate { } class A2<T> where T : MulticastDelegate { } class B2<T> : A2<T> where T : Delegate { } ``` #Resolved

OmarTawfik · 2018-01-25T22:40:54Z

@CyrusNajmabadi I believe the reason is that this is not a main scenario, and it was not worth it to do changes to the parser/language rules to enable this. This change aims at doing the least amount of work possible to unblock people from using a feature that roslyn already supports (we import and can use such generics, we just don't allow them in source).
If enough users are pushing for the language change as well, we can evaluate it.

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

CyrusNajmabadi · 2018-01-25T22:52:45Z

This change aims at doing the least amount of work possible to unblock people from using a feature that roslyn already support

IMO, while 'least amount of work' is nice from a prototyping standpoint, it's not great from an actual language design standpoint. This new language doesn't feel (to me) like how i would think C# should expose this sort of functionality. For example, the : struct constraint is not : System.ValueType.

and it was not worth it to do changes to the parser

The parser changes seem fairly trivial. I would just allow "enum/delegate" and make appropriate constraint nodes for them. Am i missing something in terms of how hard that would be. It seems tiny.

/language rules to enable this.

If we're not willing to update the language rules to allow for this in such a tiny manner... i'm wondering why we're allowing htis at all :D

CyrusNajmabadi · 2018-01-25T22:54:49Z

Don't you just need to change ParseTypeParameterConstraint into:

                case SyntaxKind.StructKeyword:
                    var structToken = this.EatToken();
                    return _syntaxFactory.ClassOrStructConstraint(SyntaxKind.StructConstraint, structToken);
                case SyntaxKind.ClassKeyword:
                    var classToken = this.EatToken();
                    return _syntaxFactory.ClassOrStructConstraint(SyntaxKind.ClassConstraint, classToken);
                case SyntaxKind.EnumKeyword:
                    var classToken = this.EatToken();
                    return _syntaxFactory.EnumConstraint(classToken);
                case SyntaxKind.DelegateKeyword:
                    var classToken = this.EatToken();
                    return _syntaxFactory.DelegateConstraint(classToken);

?

OmarTawfik · 2018-01-26T00:12:33Z

@CyrusNajmabadi

The parser changes seem fairly trivial. I would just allow "enum/delegate" and make appropriate constraint nodes for them. Am i missing something in terms of how hard that would be. It seems tiny.

Not just allowing it in the LanguageParser, but supporting it all the way through the semantic model, and IDE. Completion has to provide it, highlighting has to work, etc. The numerous places we have to fix every time the syntax changes.

If we're not willing to update the language rules to allow for this in such a tiny manner... i'm wondering why we're allowing htis at all :D

The reason is to unblock the small portion of users that would use these constraints, but otherwise cannot. My two cents based on initial discussions, the proposal comments, and design meetings, is that not a lot of people would require using a where T : delegate in the first place, and the small portion would be more than happy using where T : Delegate.

I'm not saying it is impossible, I'm just saying it is a much bigger scope than the design plan (lifting one artificial compiler error that we didn't need, and add appropriate tests). Adding additional syntax because it looks nicer is an extra goal that will add a small value to a small percentage of users.

CyrusNajmabadi · 2018-01-26T00:19:33Z

is that not a lot of people would require using a where T : delegate

I agree on that. However, where T : enum has been an ongoing request since when we first added generics. Users always even pitched it that way. "Allow me to say : enum".

CyrusNajmabadi · 2018-01-26T00:21:18Z

Not just allowing it in the LanguageParser, but supporting it all the way through the semantic model, and IDE. Completion has to provide it, highlighting has to work, etc. The numerous places we have to fix every time the syntax changes.

I'll do the IDE work :). AFAICT, the only impact would be on completion, symbol display and a bit of the code-gen infrastructure. All would be quite trivial.

VSadov

LGTM

OmarTawfik · 2018-01-29T20:49:50Z

@CyrusNajmabadi another consideration is also we can think in the future of providing enum constraints to a specific type, like enum(int), etc... We don't want to lock the syntax now without proper design, but that is outside the scope of the work I'm doing now.

OmarTawfik · 2018-01-29T20:50:30Z

@dotnet/roslyn-ide can I get a code review? I added a few IDE tests, no source changes.

CyrusNajmabadi · 2018-01-29T21:06:17Z

We don't want to lock the syntax now without proper design, but that is outside the scope of the work I'm doing now.

That depends :) Is this just temp/prototype work you're doing? Or is this intended to be checked into the compiler and become part of the language proper for all time? If the latter, then i would just want the LDM to sign off on this being the way they want the concept exposed. If LDM is fine, then no objections by me. If LDM thinks keyword-syntax is appropriate, then i think that should be done.

jasonmalinowski

IDE side changes look fine. My signoff obviously doesn't count for the rest of it. 😄

OmarTawfik · 2018-01-29T23:51:23Z

@CyrusNajmabadi Type syntax is already approved by LDM for C# 7.3, keyword syntax is still TBD.

CyrusNajmabadi · 2018-01-30T01:21:06Z

Awesome. Thanks!

Support Delegate and Multicast delegates as generic type constraints

00ff1bb

OmarTawfik added the PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. label Jan 25, 2018

OmarTawfik requested review from a team as code owners January 25, 2018 06:08

cston reviewed Jan 25, 2018

View reviewed changes

OmarTawfik requested review from VSadov and a team January 25, 2018 20:04

OmarTawfik added Area-Compilers Feature - Constraints Constraints and removed PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. labels Jan 25, 2018

OmarTawfik added this to the 15.7 milestone Jan 25, 2018

More tests

b9500ca

cston approved these changes Jan 26, 2018

View reviewed changes

VSadov approved these changes Jan 26, 2018

View reviewed changes

jasonmalinowski approved these changes Jan 29, 2018

View reviewed changes

OmarTawfik merged commit c23d361 into dotnet:features/constraints Jan 29, 2018

OmarTawfik deleted the add-delegate-constraint branch January 29, 2018 23:51

tannergooding mentioned this pull request Feb 15, 2018

Add NativeLibrary class dotnet/coreclr#16409

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support Delegate and MulticastDelegate as generic type constraints #24443

Support Delegate and MulticastDelegate as generic type constraints #24443

OmarTawfik commented Jan 25, 2018 •

edited

Loading

cston Jan 25, 2018 •

edited by OmarTawfik

Loading

cston Jan 25, 2018 •

edited by OmarTawfik

Loading

CyrusNajmabadi commented Jan 25, 2018 •

edited by OmarTawfik

Loading

cston Jan 25, 2018 •

edited by OmarTawfik

Loading

cston Jan 25, 2018 •

edited by OmarTawfik

Loading

cston Jan 25, 2018 •

edited by OmarTawfik

Loading

OmarTawfik commented Jan 25, 2018 •

edited

Loading

CyrusNajmabadi commented Jan 25, 2018 •

edited by OmarTawfik

Loading

CyrusNajmabadi commented Jan 25, 2018

OmarTawfik commented Jan 26, 2018 •

edited

Loading

CyrusNajmabadi commented Jan 26, 2018

CyrusNajmabadi commented Jan 26, 2018

VSadov left a comment

OmarTawfik commented Jan 29, 2018

OmarTawfik commented Jan 29, 2018

CyrusNajmabadi commented Jan 29, 2018

jasonmalinowski left a comment

OmarTawfik commented Jan 29, 2018

CyrusNajmabadi commented Jan 30, 2018

Support Delegate and MulticastDelegate as generic type constraints #24443

Support Delegate and MulticastDelegate as generic type constraints #24443

Conversation

OmarTawfik commented Jan 25, 2018 • edited Loading

cston Jan 25, 2018 • edited by OmarTawfik Loading

Choose a reason for hiding this comment

cston Jan 25, 2018 • edited by OmarTawfik Loading

Choose a reason for hiding this comment

CyrusNajmabadi commented Jan 25, 2018 • edited by OmarTawfik Loading

cston Jan 25, 2018 • edited by OmarTawfik Loading

Choose a reason for hiding this comment

cston Jan 25, 2018 • edited by OmarTawfik Loading

Choose a reason for hiding this comment

cston Jan 25, 2018 • edited by OmarTawfik Loading

Choose a reason for hiding this comment

OmarTawfik commented Jan 25, 2018 • edited Loading

CyrusNajmabadi commented Jan 25, 2018 • edited by OmarTawfik Loading

CyrusNajmabadi commented Jan 25, 2018

OmarTawfik commented Jan 26, 2018 • edited Loading

CyrusNajmabadi commented Jan 26, 2018

CyrusNajmabadi commented Jan 26, 2018

VSadov left a comment

Choose a reason for hiding this comment

OmarTawfik commented Jan 29, 2018

OmarTawfik commented Jan 29, 2018

CyrusNajmabadi commented Jan 29, 2018

jasonmalinowski left a comment

Choose a reason for hiding this comment

OmarTawfik commented Jan 29, 2018

CyrusNajmabadi commented Jan 30, 2018

OmarTawfik commented Jan 25, 2018 •

edited

Loading

cston Jan 25, 2018 •

edited by OmarTawfik

Loading

cston Jan 25, 2018 •

edited by OmarTawfik

Loading

CyrusNajmabadi commented Jan 25, 2018 •

edited by OmarTawfik

Loading

cston Jan 25, 2018 •

edited by OmarTawfik

Loading

cston Jan 25, 2018 •

edited by OmarTawfik

Loading

cston Jan 25, 2018 •

edited by OmarTawfik

Loading

OmarTawfik commented Jan 25, 2018 •

edited

Loading

CyrusNajmabadi commented Jan 25, 2018 •

edited by OmarTawfik

Loading

OmarTawfik commented Jan 26, 2018 •

edited

Loading