-
Notifications
You must be signed in to change notification settings - Fork 22
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
Omsmith/conditional parameters fix #668
Changes from all commits
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 | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -17,6 +17,7 @@ namespace D2L.CodeStyle.Analyzers.Immutability { | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
/// </summary> | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
internal sealed partial class ImmutabilityContext { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
private readonly ImmutableDictionary<INamedTypeSymbol, ImmutableTypeInfo> m_extraImmutableTypes; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
private readonly ImmutableHashSet<ITypeParameterSymbol> m_conditionalTypeParameters; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
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. Could just be |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
// Hard code this to avoid looking up the ITypeSymbol to include it in m_extraImmutableTypes | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
private static readonly ImmutableHashSet<SpecialType> m_totallyImmutableSpecialTypes = ImmutableHashSet.Create( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@@ -40,12 +41,34 @@ internal sealed partial class ImmutabilityContext { | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
SpecialType.System_ValueType | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
private ImmutabilityContext( IEnumerable<ImmutableTypeInfo> extraImmutableTypes ) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
m_extraImmutableTypes = extraImmutableTypes | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
.ToImmutableDictionary( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
info => info.Type, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
info => info | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
private ImmutabilityContext( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
ImmutableDictionary<INamedTypeSymbol, ImmutableTypeInfo> extraImmutableTypes, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
ImmutableHashSet<ITypeParameterSymbol> conditionalTypeParamemters | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
m_extraImmutableTypes = extraImmutableTypes; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
m_conditionalTypeParameters = conditionalTypeParamemters; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
/// <summary> | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
/// This function is intended to provide a new immutability context that considers the conditional type parameters | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
/// of the ConditionallyImmutable type being checked as immutable. It is important this is only used in those cases, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
/// and not for instance while validating type arguments to an [Immutable] type parameter. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
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. Not used for the linked function, which keeps conditional type parameters from being passed to strictly immutable ones as arguments. D2L.CodeStyle/src/D2L.CodeStyle.Analyzers/Immutability/ImmutabilityAnalyzer.cs Lines 128 to 162 in 8baddd9
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 would be "nice" if this wasn't sound only because of two different parts of analysis covering for each other, but this is pretty straight forward and doesn't require a concept like "Conditional" vs "Definite" immutability |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
/// </summary> | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
/// <param name="type">The ConditionallyImmutable type definition being checked</param> | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
/// <returns></returns> | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
public ImmutabilityContext WithConditionalTypeParametersAsImmutable( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
INamedTypeSymbol type | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
if( !Attributes.Objects.ConditionallyImmutable.IsDefined( type ) ) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
throw new InvalidOperationException( $"{nameof( WithConditionalTypeParametersAsImmutable )} should only be called on ConditionallyImmutable types" ); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
var conditionalTypeParameters = type.TypeParameters.Where( p => Attributes.Objects.OnlyIf.IsDefined( p ) ); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
return new ImmutabilityContext( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
extraImmutableTypes: m_extraImmutableTypes, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
conditionalTypeParamemters: m_conditionalTypeParameters.Union( conditionalTypeParameters ) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
j3parker marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
/// <summary> | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@@ -138,6 +161,10 @@ out diagnostic | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
return true; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
if( type is ITypeParameterSymbol tp && m_conditionalTypeParameters.Contains( tp ) ) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
return true; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
diagnostic = Diagnostic.Create( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Diagnostics.TypeParameterIsNotKnownToBeImmutable, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
getLocation(), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@@ -218,10 +245,6 @@ ITypeSymbol type | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
return ImmutableTypeKind.Total; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
if( Attributes.Objects.OnlyIf.IsDefined( type ) ) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
return ImmutableTypeKind.Total; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
if ( Attributes.Objects.ImmutableBaseClass.IsDefined( type ) ) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
return ImmutableTypeKind.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.
Might be some problem here with say...
But I haven't thought about that much.
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.
Open an issue to add test cases?
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.
#670