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

Omsmith/conditional parameters fix #668

Merged
merged 3 commits into from
Dec 1, 2020
Merged

Omsmith/conditional parameters fix #668

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
64ff463
move dictionary into ImmutabilityContext.Create
omsmith Nov 30, 2020
f0e9186
only consider [OnlyIf] parameters as immutable during definition chec…
omsmith Nov 30, 2020
13e3662
Update src/D2L.CodeStyle.Analyzers/Immutability/ImmutabilityContext.cs
omsmith Dec 1, 2020
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
4 changes: 4 additions & 0 deletions src/D2L.CodeStyle.Analyzers/Immutability/ImmutabilityAnalyzer.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,10 @@ INamedTypeSymbol typeSymbol
return;
}

if( Attributes.Objects.ConditionallyImmutable.IsDefined( typeSymbol ) ) {
immutabilityContext = immutabilityContext.WithConditionalTypeParametersAsImmutable( typeSymbol );
Copy link
Contributor Author

@omsmith omsmith Nov 30, 2020 •
edited
Loading

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

[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.

Copy link
Member

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

#670

}

ImmutableDefinitionChecker checker = new ImmutableDefinitionChecker(
compilation: ctx.Compilation,
diagnosticSink: ctx.ReportDiagnostic,
Expand Down
9 changes: 6 additions & 3 deletions src/D2L.CodeStyle.Analyzers/Immutability/ImmutabilityContext.Factory.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,7 @@ internal partial class ImmutabilityContext {
internal static ImmutabilityContext Create( Compilation compilation ) {
ImmutableDictionary<string, IAssemblySymbol> compilationAssmeblies = GetCompilationAssemblies( compilation );

var builder = ImmutableArray.CreateBuilder<ImmutableTypeInfo>( DefaultExtraTypes.Length );
var builder = ImmutableDictionary.CreateBuilder<INamedTypeSymbol, ImmutableTypeInfo>();

foreach( ( string typeName, string qualifiedAssembly ) in DefaultExtraTypes ) {
INamedTypeSymbol type;
Expand All @@ -112,10 +112,13 @@ internal partial class ImmutabilityContext {
type
);

builder.Add( info );
builder.Add( type, info );
}

return new ImmutabilityContext( builder.ToImmutable() );
return new ImmutabilityContext(
extraImmutableTypes: builder.ToImmutable(),
conditionalTypeParamemters: ImmutableHashSet<ITypeParameterSymbol>.Empty
);
}

private static ImmutableDictionary<string, IAssemblySymbol> GetCompilationAssemblies( Compilation compilation ) {
Expand Down
43 changes: 33 additions & 10 deletions src/D2L.CodeStyle.Analyzers/Immutability/ImmutabilityContext.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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


// Hard code this to avoid looking up the ITypeSymbol to include it in m_extraImmutableTypes
private static readonly ImmutableHashSet<SpecialType> m_totallyImmutableSpecialTypes = ImmutableHashSet.Create(
Expand All @@ -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.
Copy link
Contributor Author

@omsmith omsmith Nov 30, 2020 •
edited
Loading

Choose a reason for hiding this comment

The 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

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 );
}
}
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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>
Expand Down Expand Up @@ -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(),
Expand Down Expand Up @@ -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;
}
Expand Down
Loading