-
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
Two bug fixes - disallow "new (int, int)()" and induce a failure if ValueTuple is not a struct. #14161
Changes from all commits
7b5e3d3
bc1ad9d
593bddf
3f2436b
d21d9f7
732407b
3f7aac6
2b55e5e
430ffc7
f79bea9
73d4028
11355bd
3eacdfd
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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>(); | ||
|
||
private readonly NoPia.EmbeddedTypesManager _embeddedTypesManagerOpt; | ||
public override NoPia.EmbeddedTypesManager EmbeddedTypesManagerOpt | ||
|
@@ -790,6 +790,7 @@ internal Cci.INamedTypeReference Translate( | |
{ | ||
Debug.Assert(!needDeclaration); | ||
namedTypeSymbol = namedTypeSymbol.TupleUnderlyingType; | ||
CheckTupleUnderlying(namedTypeSymbol, syntaxNodeOpt, diagnostics); | ||
} | ||
|
||
// Substitute error types with a special singleton object. | ||
|
@@ -890,6 +891,23 @@ internal Cci.INamedTypeReference Translate( | |
return namedTypeSymbol; | ||
} | ||
|
||
private void CheckTupleUnderlying(NamedTypeSymbol namedTypeSymbol, SyntaxNode syntaxNodeOpt, DiagnosticBag diagnostics) | ||
{ | ||
// check that underlying type of a ValueTuple is indeed a value type (or error) | ||
// this should never happen, in theory, | ||
// 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 commentThe reason will be displayed to describe this comment to others. Learn more.
Is there a test that backs this change? #Closed There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe 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) |
||
{ | ||
// 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 commentThe reason will be displayed to describe this comment to others. Learn more.
Do we have a test for a nested case. #Closed There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe 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 |
||
} | ||
} | ||
} | ||
|
||
public static bool IsGenericType(NamedTypeSymbol toCheck) | ||
{ | ||
while ((object)toCheck != null) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 commentThe 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 commentThe 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) |
||
{ | ||
objectCreation = this.AddError(objectCreation, type, ErrorCode.ERR_NewWithTupleTypeSyntax); | ||
} | ||
|
||
return objectCreation; | ||
} | ||
} | ||
|
||
|
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