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

Avoid resolving parameter.Type in ReportParameterErrors() #63142

Merged
merged 4 commits into from
Aug 9, 2022
Merged

Avoid resolving parameter.Type in ReportParameterErrors() #63142

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
88d54d4
Avoid resolving parameter.Type in ReportParameterErrors()
cston Aug 2, 2022
6a8b194
Add comment
cston Aug 2, 2022
e9441ae
Merge remote-tracking branch 'upstream/main' into 63140
cston Aug 3, 2022
89d86f0
Merge remote-tracking branch 'upstream/main' into 63140
cston Aug 9, 2022
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
2 changes: 1 addition & 1 deletion src/Compilers/CSharp/Portable/Symbols/Source/ParameterHelpers.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -648,7 +648,7 @@ private static void ReportParameterErrors(
diagnostics.Add(ErrorCode.ERR_MethodArgCantBeRefAny, parameterSyntax.Location, parameter.Type);
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it possible for us to avoid using parameter.Type here as well?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think we're ok here because we've already checked that parameter.TypeWithAnnotations is a restricted type. That means the type is not a type parameter which is the only case where we'd have an unresolved type.

Copy link
Contributor

Choose a reason for hiding this comment

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

Will this break when we add allow T : ref struct? No need to adjust the implementation based on something we haven't done yet, but would be good to have a sense of what our path forward would be in that case.

Copy link
Member Author

Choose a reason for hiding this comment

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

Will this break when we add allow T : ref struct?

Yes, this will break when we support ref struct types as type arguments.

}

if (parameter.Scope == DeclarationScope.ValueScoped && !parameter.Type.IsErrorTypeOrRefLikeType())
if (parameter.Scope == DeclarationScope.ValueScoped && !parameter.TypeWithAnnotations.IsRefLikeType())
Copy link
Member

@jcouv jcouv Aug 2, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Is there a way to also check for the error type case without pulling too aggressively on lazy .Type? #Closed

Copy link
Member

Choose a reason for hiding this comment

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

Consider adding a comment at the top of this method that pulling on parameter.Type is cycle-prone.

Copy link
Member Author

@cston cston Aug 2, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Is there a way to also check for the error type case without pulling too aggressively on lazy .Type?

We could add a similar IsErrorType() method to TypeWithAnnotations, but I'm not sure it's worth it. (Should an unresolved type always return false for IsErrorType()?)

Consider adding a comment ...

I've added a comment to the method to avoid using parameter.Type.

{
diagnostics.Add(ErrorCode.ERR_ScopedRefAndRefStructOnly, parameterSyntax.Location);
}
Expand Down
5 changes: 5 additions & 0 deletions src/Compilers/CSharp/Portable/Symbols/TypeWithAnnotations.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -267,6 +267,8 @@ public bool IsVoidType() =>
_extensions.IsVoid(DefaultType);
public bool IsSZArray() =>
_extensions.IsSZArray(DefaultType);
public bool IsRefLikeType() =>
_extensions.IsRefLikeType(DefaultType);
public bool IsStatic =>
_extensions.IsStatic(DefaultType);
public bool IsRestrictedType(bool ignoreSpanLikeTypes = false) =>
Expand Down Expand Up @@ -840,6 +842,7 @@ internal static Extensions CreateLazy(CSharpCompilation compilation, TypeWithAnn
internal abstract bool IsStatic(TypeSymbol typeSymbol);
internal abstract bool IsVoid(TypeSymbol typeSymbol);
internal abstract bool IsSZArray(TypeSymbol typeSymbol);
internal abstract bool IsRefLikeType(TypeSymbol typeSymbol);

internal abstract TypeWithAnnotations WithTypeAndModifiers(TypeWithAnnotations type, TypeSymbol typeSymbol, ImmutableArray<CustomModifier> customModifiers);

Expand Down Expand Up @@ -869,6 +872,7 @@ public NonLazyType(ImmutableArray<CustomModifier> customModifiers)
internal override bool IsStatic(TypeSymbol typeSymbol) => typeSymbol.IsStatic;
internal override bool IsVoid(TypeSymbol typeSymbol) => typeSymbol.IsVoidType();
internal override bool IsSZArray(TypeSymbol typeSymbol) => typeSymbol.IsSZArray();
internal override bool IsRefLikeType(TypeSymbol typeSymbol) => typeSymbol.IsRefLikeType;

internal override TypeSymbol GetNullableUnderlyingTypeOrSelf(TypeSymbol typeSymbol) => typeSymbol.StrippedType();

Expand Down Expand Up @@ -936,6 +940,7 @@ public LazyNullableTypeParameter(CSharpCompilation compilation, TypeWithAnnotati

internal override bool IsVoid(TypeSymbol typeSymbol) => false;
internal override bool IsSZArray(TypeSymbol typeSymbol) => false;
internal override bool IsRefLikeType(TypeSymbol typeSymbol) => false;
internal override bool IsStatic(TypeSymbol typeSymbol) => false;

private TypeSymbol GetResolvedType()
Expand Down
36 changes: 36 additions & 0 deletions src/Compilers/CSharp/Test/Semantic/Semantics/RefFieldTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8950,6 +8950,9 @@ static void F1(scoped Unknown x, scoped R<Unknown> y)
}";
var comp = CreateCompilation(source);
comp.VerifyDiagnostics(
// (4,20): error CS9048: The 'scoped' modifier can be used for refs and ref struct values only.
// static void F1(scoped Unknown x, scoped R<Unknown> y)
Diagnostic(ErrorCode.ERR_ScopedRefAndRefStructOnly, "scoped Unknown x").WithLocation(4, 20),
// (4,27): error CS0246: The type or namespace name 'Unknown' could not be found (are you missing a using directive or an assembly reference?)
// static void F1(scoped Unknown x, scoped R<Unknown> y)
Diagnostic(ErrorCode.ERR_SingleTypeNameNotFound, "Unknown").WithArguments("Unknown").WithLocation(4, 27),
Expand Down Expand Up @@ -10557,6 +10560,39 @@ static void M(R<T> r) { }
Diagnostic(ErrorCode.WRN_UnreferencedField, "F").WithArguments("R<T>.F").WithLocation(4, 19));
}

[WorkItem(63140, "https://github.com/dotnet/roslyn/issues/63140")]
[Fact]
public void Scoped_Cycle()
{
var source =
@"interface I
{
void M<T>(T s);
}

class C : I
{
void I.M<T>(scoped T? s) {}
}";
var comp = CreateCompilation(source);
comp.VerifyDiagnostics(
// (6,11): error CS0535: 'C' does not implement interface member 'I.M<T>(T)'
// class C : I
Diagnostic(ErrorCode.ERR_UnimplementedInterfaceMember, "I").WithArguments("C", "I.M<T>(T)").WithLocation(6, 11),
// (8,12): error CS0539: 'C.M<T>(T?)' in explicit interface declaration is not found among members of the interface that can be implemented
// void I.M<T>(scoped T? s) {}
Diagnostic(ErrorCode.ERR_InterfaceMemberNotFound, "M").WithArguments("C.M<T>(T?)").WithLocation(8, 12),
// (8,17): error CS9048: The 'scoped' modifier can be used for refs and ref struct values only.
// void I.M<T>(scoped T? s) {}
Diagnostic(ErrorCode.ERR_ScopedRefAndRefStructOnly, "scoped T? s").WithLocation(8, 17),
// (8,25): warning CS8632: The annotation for nullable reference types should only be used in code within a '#nullable' annotations context.
// void I.M<T>(scoped T? s) {}
Diagnostic(ErrorCode.WRN_MissingNonNullTypesContextForAnnotation, "?").WithLocation(8, 25),
// (8,27): error CS0453: The type 'T' must be a non-nullable value type in order to use it as parameter 'T' in the generic type or method 'Nullable<T>'
// void I.M<T>(scoped T? s) {}
Diagnostic(ErrorCode.ERR_ValConstraintNotSatisfied, "s").WithArguments("System.Nullable<T>", "T", "T").WithLocation(8, 27));
}

[Fact]
public void NestedScope()
{
Expand Down