Omsmith/conditional parameters fix #668

omsmith · 2020-11-30T20:54:19Z

No description provided.

…king

omsmith · 2020-11-30T20:56:23Z

src/D2L.CodeStyle.Analyzers/Immutability/ImmutabilityAnalyzer.cs

@@ -116,6 +116,10 @@ INamedTypeSymbol typeSymbol
 				return;
 			}

+			if( Attributes.Objects.ConditionallyImmutable.IsDefined( typeSymbol ) ) {
+				immutabilityContext = immutabilityContext.WithConditionalTypeParametersAsImmutable( typeSymbol );


Might be some problem here with say...

[ConditionallyImmutable] public sealed class Foo<[ConditionallyImmutable.OnlyIf] T> { [ConditionallyImmutable] public sealed class Bar<[ConditionallyImmutable.OnlyIf] U> { // something in here.... } }

But I haven't thought about that much.

Open an issue to add test cases?

omsmith · 2020-11-30T20:57:52Z

src/D2L.CodeStyle.Analyzers/Immutability/ImmutabilityContext.cs

+		/// <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.


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

private static void AnalyzeGenericMethodTypeArguments(

SyntaxNodeAnalysisContext ctx,

ImmutabilityContext immutabilityContext,

GenericNameSyntax syntax

) {

if( syntax.IsFromDocComment() ) {

// ignore things in doccomments such as crefs

return;

}

SymbolInfo info = ctx.SemanticModel.GetSymbolInfo( syntax, ctx.CancellationToken );

var (typeParameters, typeArguments) = GetTypeParamsAndArgs( info.Symbol );

int i = 0;

var paramArgPairs = typeParameters.Zip( typeArguments, ( p, a ) => (p, a, i++) );

foreach( var (parameter, argument, position) in paramArgPairs ) {

// TODO: this should eventually use information from ImmutableTypeInfo

// however the current information about immutable type parameters

// includes [Immutable] filling for what will instead be the upcoming

// [OnlyIf] (e.g. it would be broken for IEnumerable<>)

if( !Attributes.Objects.Immutable.IsDefined( parameter ) ) {

continue;

}

if( !immutabilityContext.IsImmutable(

type: argument,

kind: ImmutableTypeKind.Total,

getLocation: () => syntax.TypeArgumentList.Arguments[position].GetLocation(),

out Diagnostic diagnostic

) ) {

// TODO: not necessarily a good diagnostic for this use-case

ctx.ReportDiagnostic( diagnostic );

}

}

}

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

omsmith · 2020-11-30T20:58:58Z

src/D2L.CodeStyle.Analyzers/Immutability/ImmutabilityContext.cs

@@ -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;


Could just be ITypeSymbol and not parameter specific,, but 🤷 can be adjusted later if needed.

src/D2L.CodeStyle.Analyzers/Immutability/ImmutabilityContext.cs

omsmith · 2020-11-30T21:01:34Z

tests/D2L.CodeStyle.Analyzers.Test/Specs/ImmutabilityAnalyzer.cs

@@ -769,17 +769,17 @@ public sealed class AnalyzedImmutableGenericClassGivenT<[ConditionallyImmutable.



-		static T /* MemberIsNotReadOnly(Field, m_field, AnalyzedImmutableGenericClassGivenT) */ m_field /**/;
-		static readonly T m_field;
+		static /* TypeParameterIsNotKnownToBeImmutable(T) */ T /**/ /* MemberIsNotReadOnly(Field, m_field, AnalyzedImmutableGenericClassGivenT) */ m_field /**/;


The conditional parameters aren't added as immutable when verifying statics. They aren't safe to hold as static, only definitely immutable things are.

Note that these updates are in the context of

[ConditionallyImmutable] public sealed class AnalyzedImmutableGenericClassGivenT<[ConditionallyImmutable.OnlyIf] T, U> {

omsmith · 2020-11-30T21:03:32Z

tests/D2L.CodeStyle.Analyzers.Test/Specs/ImmutabilityAnalyzer.cs

-		static /* TypeParameterIsNotKnownToBeImmutable(U) */ Types.SomeImmutableGenericInterfaceGivenTU<T, U> /**/ /* MemberIsNotReadOnly(Field, m_field, AnalyzedImmutableGenericClassGivenT) */ m_field /**/;
-		static readonly /* TypeParameterIsNotKnownToBeImmutable(U) */ Types.SomeImmutableGenericInterfaceGivenTU<T, U> /**/ m_field;
+		static /* TypeParameterIsNotKnownToBeImmutable(T) */ Types.SomeImmutableGenericInterfaceGivenTU<T, U> /**/ /* MemberIsNotReadOnly(Field, m_field, AnalyzedImmutableGenericClassGivenT) */ m_field /**/;
+		static readonly /*  TypeParameterIsNotKnownToBeImmutable(T) */ Types.SomeImmutableGenericInterfaceGivenTU<T, U> /**/ m_field;


I expected this to be TypeParameterIsNotKnownToBeImmutable(T), TypeParameterIsNotKnownToBeImmutable(U), but these diagnostics come from immutableTypeInfo.IsImmutableDefinition, and the result and diagnostic come from the first parameter that's an issue (i.e. it's out Diagnostic, not out IEnumerable<Diagnostic>)

omsmith · 2020-11-30T21:05:14Z

tests/D2L.CodeStyle.Analyzers.Test/Specs/ImmutabilityAnalyzer.cs

-		readonly Types.SomeImmutableGenericInterfaceRestrictingT<T, U> m_field;
-		Types.SomeImmutableGenericInterfaceRestrictingT<T, U> Property { get; }
-		Types.SomeImmutableGenericInterfaceRestrictingT<T, U> Property { get { return default; } }
+		static Types.SomeImmutableGenericInterfaceRestrictingT</* TypeParameterIsNotKnownToBeImmutable(T) */ T /**/, U> /* MemberIsNotReadOnly(Field, m_field, AnalyzedImmutableGenericClassGivenT) */ m_field /**/;


conditional type parameters aren't considered immutable when checking arguments to immutable type parameters (<[Immutable] T>). These parameters are expecting definitely immutable things.

omsmith · 2020-11-30T21:06:51Z

It may be a little confusing to be told "T" isn't considered immutable for a static, for instance, but not for trying to hold it directly, but... we'll see how it plays out and address via #650

omsmith · 2020-11-30T21:07:08Z

Done commenting

src/D2L.CodeStyle.Analyzers/Immutability/ImmutabilityContext.cs

Co-authored-by: Jacob Parker <[email protected]>

omsmith added 2 commits November 30, 2020 14:51

move dictionary into ImmutabilityContext.Create

64ff463

only consider [OnlyIf] parameters as immutable during definition chec…

f0e9186

…king

omsmith requested review from j3parker and mthjones as code owners November 30, 2020 20:54

omsmith commented Nov 30, 2020

View reviewed changes

src/D2L.CodeStyle.Analyzers/Immutability/ImmutabilityContext.cs Show resolved Hide resolved

omsmith commented Nov 30, 2020

View reviewed changes

j3parker reviewed Dec 1, 2020

View reviewed changes

src/D2L.CodeStyle.Analyzers/Immutability/ImmutabilityContext.cs Outdated Show resolved Hide resolved

Update src/D2L.CodeStyle.Analyzers/Immutability/ImmutabilityContext.cs

13e3662

Co-authored-by: Jacob Parker <[email protected]>

omsmith mentioned this pull request Dec 1, 2020

Consider conditional immutability of nested classes #670

Open

j3parker approved these changes Dec 1, 2020

View reviewed changes

omsmith merged commit 954ef6d into master Dec 1, 2020

omsmith deleted the omsmith/conditional-parameters-fix branch December 1, 2020 18:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Omsmith/conditional parameters fix #668

Omsmith/conditional parameters fix #668

omsmith commented Nov 30, 2020

omsmith Nov 30, 2020 •

edited

Loading

j3parker Dec 1, 2020

omsmith Dec 1, 2020

omsmith Nov 30, 2020 •

edited

Loading

omsmith Nov 30, 2020

omsmith Nov 30, 2020

omsmith Nov 30, 2020 •

edited

Loading

omsmith Nov 30, 2020 •

edited

Loading

omsmith Nov 30, 2020 •

edited

Loading

omsmith Nov 30, 2020 •

edited

Loading

omsmith commented Nov 30, 2020 •

edited

Loading

omsmith commented Nov 30, 2020

	private static void AnalyzeGenericMethodTypeArguments(
	SyntaxNodeAnalysisContext ctx,
	ImmutabilityContext immutabilityContext,
	GenericNameSyntax syntax
	) {
	if( syntax.IsFromDocComment() ) {
	// ignore things in doccomments such as crefs
	return;
	}

	SymbolInfo info = ctx.SemanticModel.GetSymbolInfo( syntax, ctx.CancellationToken );
	var (typeParameters, typeArguments) = GetTypeParamsAndArgs( info.Symbol );

	int i = 0;
	var paramArgPairs = typeParameters.Zip( typeArguments, ( p, a ) => (p, a, i++) );
	foreach( var (parameter, argument, position) in paramArgPairs ) {
	// TODO: this should eventually use information from ImmutableTypeInfo
	// however the current information about immutable type parameters
	// includes [Immutable] filling for what will instead be the upcoming
	// [OnlyIf] (e.g. it would be broken for IEnumerable<>)
	if( !Attributes.Objects.Immutable.IsDefined( parameter ) ) {
	continue;
	}

	if( !immutabilityContext.IsImmutable(
	type: argument,
	kind: ImmutableTypeKind.Total,
	getLocation: () => syntax.TypeArgumentList.Arguments[position].GetLocation(),
	out Diagnostic diagnostic
	) ) {
	// TODO: not necessarily a good diagnostic for this use-case
	ctx.ReportDiagnostic( diagnostic );
	}
	}
	}

Omsmith/conditional parameters fix #668

Omsmith/conditional parameters fix #668

Conversation

omsmith commented Nov 30, 2020

omsmith Nov 30, 2020 • edited Loading

Choose a reason for hiding this comment

j3parker Dec 1, 2020

Choose a reason for hiding this comment

omsmith Dec 1, 2020

Choose a reason for hiding this comment

omsmith Nov 30, 2020 • edited Loading

Choose a reason for hiding this comment

omsmith Nov 30, 2020

Choose a reason for hiding this comment

omsmith Nov 30, 2020

Choose a reason for hiding this comment

omsmith Nov 30, 2020 • edited Loading

Choose a reason for hiding this comment

omsmith Nov 30, 2020 • edited Loading

Choose a reason for hiding this comment

omsmith Nov 30, 2020 • edited Loading

Choose a reason for hiding this comment

omsmith Nov 30, 2020 • edited Loading

Choose a reason for hiding this comment

omsmith commented Nov 30, 2020 • edited Loading

omsmith commented Nov 30, 2020

omsmith Nov 30, 2020 •

edited

Loading

omsmith Nov 30, 2020 •

edited

Loading

omsmith Nov 30, 2020 •

edited

Loading

omsmith Nov 30, 2020 •

edited

Loading

omsmith Nov 30, 2020 •

edited

Loading

omsmith Nov 30, 2020 •

edited

Loading

omsmith commented Nov 30, 2020 •

edited

Loading