-
Notifications
You must be signed in to change notification settings - Fork 4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Two bug fixes - disallow "new (int, int)()" and induce a failure if ValueTuple is not a struct. #14161
Conversation
@@ -3181,6 +3181,12 @@ protected BoundExpression BindObjectCreationExpression(ObjectCreationExpressionS | |||
{ | |||
var type = BindType(node.Type, diagnostics); | |||
|
|||
if (node.Type.Kind() == SyntaxKind.TupleType) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please move this constraint into the parser? It is likely to be specified as a syntactic constraint. The syntax new (x, y)
is reasonably likely to be a work item in the not too distant future (#35). Having this handled in the parser would make the next person's job much easier. #Resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will try move it to parser. #Resolved
@@ -21,7 +21,7 @@ internal abstract class PEModuleBuilder : PEModuleBuilder<CSharpCompilation, Sou | |||
// TODO: Need to estimate amount of elements for this map and pass that value to the constructor. | |||
protected readonly ConcurrentDictionary<Symbol, Cci.IModuleReference> AssemblyOrModuleSymbolToModuleRefMap = new ConcurrentDictionary<Symbol, Cci.IModuleReference>(); | |||
private readonly ConcurrentDictionary<Symbol, object> _genericInstanceMap = new ConcurrentDictionary<Symbol, object>(); | |||
private readonly ConcurrentSet<ErrorTypeSymbol> _reportedErrorTypesMap = new ConcurrentSet<ErrorTypeSymbol>(); | |||
private readonly ConcurrentSet<TypeSymbol> _reportedErrorTypesMap = new ConcurrentSet<TypeSymbol>(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this set should use an identity comparer. #Resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will change to identity.
Note that this is an optimization for the number of errors spitted out in extremely rare and badly broken cases. I do not think it makes a lot of difference either way. #Resolved
@dotnet-bot test perf_correctness_prtest please |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good as far as it goes. However I believe we should also forbid new'ing a nullable tuple type as well, i.e. new (int, int)?()
should be an error. Such an expression can be replaced by null
.
@@ -10676,7 +10676,13 @@ private ExpressionSyntax ParseArrayOrObjectCreationExpression() | |||
SyntaxFactory.MissingToken(SyntaxKind.CloseParenToken)); | |||
} | |||
|
|||
return _syntaxFactory.ObjectCreationExpression(@new, type, argumentList, initializer); | |||
var objectCreation = _syntaxFactory.ObjectCreationExpression(@new, type, argumentList, initializer); | |||
if (type.Kind == SyntaxKind.TupleType) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might be nicer to put the error on the tuple type. #ByDesign
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried to figure what is at fault here - the "new" or the tuple literal. And ended up deciding to flag the whole thing since it is the combination that is invalid.
In reply to: 81399515 [](ancestors = 81399515)
@@ -2597,7 +2597,7 @@ class C1 | |||
{ | |||
static void Test(int arg1, (byte, byte) arg2) | |||
{ | |||
(int, int) t1 = new(int, int)(); | |||
(int, int)? t1 = new(int, int)?(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please modify the test to assert what the diagnostics are? This syntax should also not work (i.e. a nullable tuple type syntax with new
should also be a syntax error). #Resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about new (int, int)[0] t1
or new (int, int)?[0] t1
? #ByDesign
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we need more design discussions for additional restrictions.
So far we agreed to not allow new
directly with tuple types.
This change implements that decision.
It started with not being able to do that anyways for tuple types with 7+ elements. With nullables or arrays there are no problems on their own, so we need to formalize another motivation.
We should discuss addition restrictions more comprehensively, just to be sure there is a common rational reason for all the cases and that we covered them all.
#WontFix
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to mention another case:
new (int, int)?(<value>)
is also legal
Nullable<T>
has a ctor that takes T, so as long as <value>
converts to T, the code above has meaning.
So will need to decide whether to allow it or not.
#WontFix
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Per current rules only a "new" applied immediately to a tuple type is not permitted. So this is valid. For now.
I am afraid generalizing the rule to a wider range of syntax is not so straightforward and unambiguous. I expect a range of opinions and valid reasons why we need to go one way or another from here, so I am not brave enough to guess :-)
If we want to cover a wider range of syntax, we need to tweak the design and implement it in another change. #WontFix
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, but please do check the specific diagnostics here. #Resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test does not result in any diagnostics. (only valid cases are here) and there is validation that we do not have diagnostics.
Once some of this cases become errors, the test will catch that. #Resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I mistakenly thought it was asserting there were errors.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wont Fix for this PR.
We need a better understanding of desired design for this change
In reply to: 81405544 [](ancestors = 81405544)
if (node.Type.Kind() == SyntaxKind.TupleType) | ||
{ | ||
Debug.Assert(node.HasErrors, "new <tuple type> should be a syntax error"); | ||
return BadExpression(node, LookupResultKind.NotCreatable); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should type
be used for the type? #WontFix
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Considering that it might parse into something else in the future, I do not want to bind the type. #WontFix
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Binding the type might result in more diagnostics, or opposite - something that uses the expression type may actually work. And that will change once we change parsing. It seems to be better if we do not bind. #Resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aren't we already binding the type at line 3182? #Resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah. I see. I meant to move this to after the check. Somehow type binding is still there. Will fix.
In reply to: 81443846 [](ancestors = 81443846)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -21,7 +21,7 @@ internal abstract class PEModuleBuilder : PEModuleBuilder<CSharpCompilation, Sou | |||
// TODO: Need to estimate amount of elements for this map and pass that value to the constructor. | |||
protected readonly ConcurrentDictionary<Symbol, Cci.IModuleReference> AssemblyOrModuleSymbolToModuleRefMap = new ConcurrentDictionary<Symbol, Cci.IModuleReference>(); | |||
private readonly ConcurrentDictionary<Symbol, object> _genericInstanceMap = new ConcurrentDictionary<Symbol, object>(); | |||
private readonly ConcurrentSet<ErrorTypeSymbol> _reportedErrorTypesMap = new ConcurrentSet<ErrorTypeSymbol>(); | |||
private readonly ConcurrentSet<TypeSymbol> _reportedErrorTypesMap = new ConcurrentSet<TypeSymbol>(ReferenceEqualityComparer.Instance); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why use reference equality rather than Equals
? Won't we report more errors as a result? #ByDesign
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are always original types or error types, which are equal by identity. #Resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did not realize that, just followed Neal's advise, but yes, they are definitions.
In reply to: 81423689 [](ancestors = 81423689)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general, error types are not equal by identity. Please revert the change.
In reply to: 81434867 [](ancestors = 81434867,81423689)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -2597,7 +2597,7 @@ class C1 | |||
{ | |||
static void Test(int arg1, (byte, byte) arg2) | |||
{ | |||
(int, int) t1 = new(int, int)(); | |||
(int, int)? t1 = new(int, int)?(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I mistakenly thought it was asserting there were errors.
@dotnet/roslyn-compiler PING. |
{ | ||
diagnostics.Add(new CSDiagnostic(new CSDiagnosticInfo(ErrorCode.ERR_PredefinedValueTupleTypeMustBeStruct, namedTypeSymbol.MetadataName), syntaxNodeOpt == null ? NoLocation.Singleton : syntaxNodeOpt.Location)); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider extracting this block to a method.
Also, there may be a simpler way to report the diagnostic. #Resolved
// this should never happen, in theory, | ||
// but if it does happen we should make it a failure. | ||
var declaredBase = namedTypeSymbol.BaseTypeNoUseSiteDiagnostics; | ||
if (declaredBase.SpecialType != SpecialType.System_ValueType && !declaredBase.IsErrorType()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason not to simply check namedTypeSymbol.IsValueType
? Note that TupleTypeSymbol.IsValueType
already defaults to true if the underlying type is an error type. #ByDesign
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
namedTypeSymbol is not necessarily a tuple and many types would default to be classes or whatever.
We do not want Enums, which are value types, so checking for a base ValueType is actually simpler and more precise.
In reply to: 81642661 [](ancestors = 81642661)
<value>Predefined type '{0}' must be a struct.</value> | ||
</data> | ||
<data name="ERR_NewWithTupleTypeSyntax" xml:space="preserve"> | ||
<value>"new" cannot be used with tuple type '{0}'. Use a tuple literal expression instead.</value> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Resource strings typically using single quotes rather than double quotes for keywords: 'new'
rather than "new"
#Resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
var objectCreation = _syntaxFactory.ObjectCreationExpression(@new, type, argumentList, initializer); | ||
if (type.Kind == SyntaxKind.TupleType) | ||
{ | ||
objectCreation = this.AddError(objectCreation, objectCreation, ErrorCode.ERR_NewWithTupleTypeSyntax, type.ToString()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we have add this error during parsing, why do we also need special handling in BindObjectCreationExpression
? #Resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By default we do attempt to bind syntax even if it has errors. We should not in his case. It is intentional that it means nothing now because we expect it to mean a different thing in the future.
In reply to: 81643102 [](ancestors = 81643102)
// we need to have some support for classes, but most common case will be struct | ||
// in broken scenarios (ErrorType, Enum, Delegate, whatever..) we will just default to struct. | ||
return _underlyingType.TypeKind == TypeKind.Class ? TypeKind.Class : TypeKind.Struct; | ||
// From the language prospective tuple is a value type |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
prospective [](start = 37, length = 11)
s/prospective/perspective/ #Resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -261,8 +261,8 @@ .maxstack 6 | |||
IL_002f: ldstr ""y"" | |||
IL_0034: call ""(int A, int B) Microsoft.VisualStudio.Debugger.Clr.IntrinsicMethods.GetVariableAddress<(int A, int B)>(string)"" | |||
IL_0039: ldloc.0 | |||
IL_003a: stind.ref | |||
IL_003b: ret | |||
IL_003a: stobj ""System.ValueTuple<int, int>"" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't understand why this IL changed. What caused this? #ByDesign
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is an error type because testcase does not reference System facade where ValueType lives. As a result ValueTuple was considered a class.
This code would not be emittable in regular compile scenario. It would have diagnostics about missing references.
In reply to: 81644864 [](ancestors = 81644864,81644123)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is really a result of the changes that makes tuples value types regardless whether backing types are errors or not.
In reply to: 81645389 [](ancestors = 81645389,81644864,81644123)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you clarify why this isn't an error in this case?
In reply to: 81645590 [](ancestors = 81645590,81645389,81644864,81644123)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Logged #14267 to report a compile error here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! We should either recognize missing ValueType to be a special type (that would make derived types structs) or report an error. Neither is happening.
In reply to: 81822455 [](ancestors = 81822455)
// (9,5): error CS8180: Predefined type 'ValueTuple`2' must be a struct. | ||
// { | ||
Diagnostic(ErrorCode.ERR_PredefinedValueTupleTypeMustBeStruct, @"{ | ||
return (1, 1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The area for squiggles seems larger than necessary. Why isn't the error just located on the tuple literal or the method return type? #ByDesign
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is what is passed to that method as a relevant syntax reference from the callers.
I think this is sufficient. This is going to be very rare diagnostics. It does not need to be very user friendly.
In reply to: 81644930 [](ancestors = 81644930)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM Thanks
// this should never happen, in theory, | ||
// but if it does happen we should make it a failure. | ||
var declaredBase = namedTypeSymbol.BaseTypeNoUseSiteDiagnostics; | ||
if (declaredBase.SpecialType != SpecialType.System_ValueType && !declaredBase.IsErrorType()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
declaredBase
will be null
for interface ValueTuple<T1, T2>
. #Resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM |
if (node.Type.Kind() == SyntaxKind.TupleType) | ||
{ | ||
Debug.Assert(node.HasErrors, "new <tuple type> should be a syntax error"); | ||
return BadExpression(node, LookupResultKind.NotCreatable); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return BadExpression(node, LookupResultKind.NotCreatable); [](start = 16, length = 58)
I think we shouldn't cancel the binding short, all constituent parts should be bound and preserved in the tree for SemanticModel benefits. I suggest we simply continue normally as it doesn't look like returning bad expression actually gives us much. #Closed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed. If we happened to parse it (not make a junk trivia), we may as well bind it.
Perhaps should put "HasError" flag on the bound node though. To make sure it does not go further.
In reply to: 81652702 [](ancestors = 81652702)
var objectCreation = _syntaxFactory.ObjectCreationExpression(@new, type, argumentList, initializer); | ||
if (type.Kind == SyntaxKind.TupleType) | ||
{ | ||
objectCreation = this.AddError(objectCreation, objectCreation, ErrorCode.ERR_NewWithTupleTypeSyntax, type.ToString()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
type.ToString() [](start = 121, length = 15)
Do we do this anywhere else? How does this look with line breaks within the type syntax? #Closed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is more common that we do "currentToken.Text" which also can be multiline.
I think the error will not be nice looking. Multiline errors are not nice looking in general.
I do not think there is a lot of choice here other than not specify the type in the error at all.
In reply to: 81653476 [](ancestors = 81653476)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Error gets truncated. It does look ugly.
I will put squigglies only for the type and not mention the actual type i the error
In reply to: 81658408 [](ancestors = 81658408,81653476)
// Try to decrease noise by not complaining about the same type over and over again. | ||
if (_reportedErrorTypesMap.Add(namedTypeSymbol)) | ||
{ | ||
diagnostics.Add(new CSDiagnostic(new CSDiagnosticInfo(ErrorCode.ERR_PredefinedValueTupleTypeMustBeStruct, namedTypeSymbol.MetadataName), syntaxNodeOpt == null ? NoLocation.Singleton : syntaxNodeOpt.Location)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
diagnostics.Add(new CSDiagnostic(new CSDiagnosticInfo(ErrorCode.ERR_PredefinedValueTupleTypeMustBeStruct, namedTypeSymbol.MetadataName), syntaxNodeOpt == null ? NoLocation.Singleton : syntaxNodeOpt.Location)); [](start = 20, length = 209)
Do we have a test for a nested case. #Closed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A tuple of cardinality >= 8 is used and VT`8 is not a structure, or "short" VT is not a structure, or both are not structures. #Resolved
Do we have a test verifying that one can still invoke constructors by using type name?
|
Yes, we have those. UnifyUnderlyingWithTuple_08 has a bunch. In reply to: 251253047 [](ancestors = 251253047) |
@dotnet-bot test windows_debug_unit32_prtest please |
// but if it does happen we should make it a failure. | ||
// NOTE: declaredBase could be null for interfaces | ||
var declaredBase = namedTypeSymbol.BaseTypeNoUseSiteDiagnostics; | ||
if (declaredBase?.SpecialType != SpecialType.System_ValueType && declaredBase?.IsErrorType() != true) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
!= true [](start = 104, length = 8)
Is there a test that backs this change? #Closed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It covers the case where ValueTuple is an interface. (and it was indeed causing a crash)
In reply to: 81813233 [](ancestors = 81813233,81812665)
Fixes: #10891 - Forbid constructing tuples via "new"
Fixes: #11689 - Enforce that ValueTuple types are structs
Note that #11689 requires validation that ValueTuple is a value type only in common cases, That is - to convey that cases where ValueTuple happens to be a Class, Enum, Delegate, etc are clearly not supported.
From the language prospective, tuples are value types composed of their elements. That is visible through the semantics of definite assignment, conversions, nullable lifting, async capturing and so on. This change intentionally introduces emit failures when underlying ValueTuple types are not structs.
This is only C# part.
VB will need exactly same fixes ported.