Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Two bug fixes - disallow "new (int, int)()" and induce a failure if ValueTuple is not a struct. #14161

Merged
merged 13 commits into from
Oct 4, 2016
Merged

Two bug fixes - disallow "new (int, int)()" and induce a failure if ValueTuple is not a struct. #14161

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
7b5e3d3
Fixes: #11689
VSadov Sep 28, 2016
bc1ad9d
Fixes: https://github.com/dotnet/roslyn/issues/10891
VSadov Sep 28, 2016
593bddf
CR feedback
VSadov Sep 29, 2016
3f2436b
Merge remote-tracking branch 'refs/remotes/upstream/master'
VSadov Sep 29, 2016
d21d9f7
Updated parser tests
VSadov Sep 29, 2016
732407b
Merge remote-tracking branch 'refs/remotes/upstream/master'
VSadov Sep 29, 2016
3f7aac6
adjust EE tests
VSadov Sep 30, 2016
2b55e5e
merged recent master
VSadov Sep 30, 2016
430ffc7
added couple small scenarios to parsing tests.
VSadov Sep 30, 2016
f79bea9
move tuple type binding in object creation past the check for a tuple…
VSadov Oct 1, 2016
73d4028
merged recent master
VSadov Oct 3, 2016
11355bd
More CR feedback
VSadov Oct 3, 2016
3eacdfd
More CR feedback
VSadov Oct 4, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions src/Compilers/CSharp/Portable/Binder/Binder_Expressions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3181,6 +3181,12 @@ protected BoundExpression BindObjectCreationExpression(ObjectCreationExpressionS
{
var type = BindType(node.Type, diagnostics);

if (node.Type.Kind() == SyntaxKind.TupleType)
Copy link
Member

@gafter gafter Sep 29, 2016 •
edited by VSadov
Loading

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

Copy link
Member Author

@VSadov VSadov Sep 29, 2016 •
edited
Loading

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

{
Error(diagnostics, ErrorCode.ERR_NewWithTupleTypeSyntax, node, type);
return BadExpression(node, LookupResultKind.NotCreatable);
Copy link
Member

@cston cston Sep 30, 2016 •
edited by VSadov
Loading

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

Copy link
Member Author

@VSadov VSadov Sep 30, 2016 •
edited
Loading

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

Copy link
Member Author

@VSadov VSadov Sep 30, 2016 •
edited
Loading

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

Copy link
Member

@cston cston Oct 1, 2016 •
edited by VSadov
Loading

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

Copy link
Member Author

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)

Copy link
Member Author

@VSadov VSadov Oct 1, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for noticing this! Fixed.

In reply to: 81451427 [](ancestors = 81451427,81443846)

Copy link
Contributor

@AlekseyTs AlekseyTs Oct 3, 2016 •
edited
Loading

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

Copy link
Member Author

@VSadov VSadov Oct 3, 2016 •
edited
Loading

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)

}

BoundExpression boundInitializerOpt = node.Initializer == null ?
null :
BindInitializerExpressionOrValue(
Expand Down
6 changes: 6 additions & 0 deletions src/Compilers/CSharp/Portable/CSharpResources.resx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4929,6 +4929,12 @@ To remove the warning, you can use /reference instead (set the Embed Interop Typ
<data name="WRN_TupleLiteralNameMismatch_Title" xml:space="preserve">
<value>The tuple element name is ignored because a different name is specified by the assignment target.</value>
</data>
<data name="ERR_PredefinedValueTupleTypeMustBeStruct" xml:space="preserve">
<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>
Copy link
Member

@cston cston Oct 3, 2016 •
edited by VSadov
Loading

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

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

</data>
<data name="ERR_DeconstructionVarFormDisallowsSpecificType" xml:space="preserve">
<value>Deconstruction 'var (...)' form disallows a specific type for 'var'.</value>
</data>
Expand Down
15 changes: 14 additions & 1 deletion src/Compilers/CSharp/Portable/Emitter/Model/PEModuleBuilder.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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>();
Copy link
Member

@gafter gafter Sep 29, 2016 •
edited by VSadov
Loading

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

Copy link
Member Author

@VSadov VSadov Sep 29, 2016 •
edited
Loading

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


private readonly NoPia.EmbeddedTypesManager _embeddedTypesManagerOpt;
public override NoPia.EmbeddedTypesManager EmbeddedTypesManagerOpt
Expand Down Expand Up @@ -790,6 +790,19 @@ internal Cci.INamedTypeReference Translate(
{
Debug.Assert(!needDeclaration);
namedTypeSymbol = namedTypeSymbol.TupleUnderlyingType;

// 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.
var declaredBase = namedTypeSymbol.BaseTypeNoUseSiteDiagnostics;
if (declaredBase.SpecialType != SpecialType.System_ValueType && !declaredBase.IsErrorType())
Copy link
Member

@jcouv jcouv Oct 3, 2016 •
edited by VSadov
Loading

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

Copy link
Member Author

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)

Copy link
Member

@cston cston Oct 3, 2016 •
edited by VSadov
Loading

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!! Fixed and added a test.

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

{
// 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));
}
}
Copy link
Member

@jcouv jcouv Oct 3, 2016 •
edited by VSadov
Loading

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

}

// Substitute error types with a special singleton object.
Expand Down
4 changes: 3 additions & 1 deletion src/Compilers/CSharp/Portable/Errors/ErrorCode.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1428,8 +1428,10 @@ internal enum ErrorCode

ERR_PredefinedValueTupleTypeNotFound = 8179,
ERR_SemiOrLBraceOrArrowExpected = 8180,
ERR_NewWithTupleTypeSyntax = 8181,
ERR_PredefinedValueTupleTypeMustBeStruct = 8182,

// Available = 8181-8195
// Available = 8183-8195

#region diagnostics for out var
ERR_ImplicitlyTypedOutVariableUsedInTheSameArgumentList = 8196,
Expand Down
23 changes: 3 additions & 20 deletions src/Compilers/CSharp/Portable/Symbols/Tuples/TupleTypeSymbol.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -663,22 +663,6 @@ internal override ImmutableArray<NamedTypeSymbol> InterfacesNoUseSiteDiagnostics
return _underlyingType.InterfacesNoUseSiteDiagnostics(basesBeingResolved);
}

public override bool IsReferenceType
{
get
{
return _underlyingType.IsErrorType() ? false : _underlyingType.IsReferenceType;
}
}

public override bool IsValueType
{
get
{
return _underlyingType.IsErrorType() || _underlyingType.IsValueType;
}
}

internal sealed override bool IsManagedType
{
get
Expand Down Expand Up @@ -1097,10 +1081,9 @@ public override TypeKind TypeKind
{
get
{
// only classes and structs can have instance fields as tuple requires.
// 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
Copy link
Member

@jcouv jcouv Oct 3, 2016 •
edited by VSadov
Loading

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

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

// composed of its underlying elements
return TypeKind.Struct;
}
}

Expand Down
Loading