Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

Conversation

333fred
Copy link
Member

@333fred 333fred commented Oct 3, 2025

Instead of synthesizing inline array types, we want to use the ones from the framework when possible (in .NET 10). Contributes to #74538.

Instead of synthesizing inline array types, we want to use the ones from the framework when possible (in .NET 10). Contributes to dotnet#74538.
@333fred 333fred requested review from a team as code owners October 3, 2025 23:34
<NetVSShared>net8.0</NetVSShared>
<NetRoslynBuildHostNetCoreVersion>net6.0</NetRoslynBuildHostNetCoreVersion>
<NetRoslynNext>net9.0</NetRoslynNext>
<NetRoslynNext>net10.0</NetRoslynNext>
Copy link
Member Author

Choose a reason for hiding this comment

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

The only projects using this value are now the Emit3 unit tests, and BuildBoss.

src/Compilers/Core/Portable/WellKnownTypes.cs Outdated Show resolved Hide resolved
src/Compilers/Core/Portable/WellKnownTypes.cs Outdated Show resolved Hide resolved
@333fred
Copy link
Member Author

333fred commented Oct 7, 2025

@dotnet/roslyn-compiler for a second review

src/Compilers/Core/Portable/WellKnownTypes.cs Show resolved Hide resolved
@AlekseyTs
Copy link
Contributor

AlekseyTs commented Oct 8, 2025

Done with review pass (commit 4) #Closed

@333fred
Copy link
Member Author

333fred commented Oct 9, 2025

@AlekseyTs for another review pass

src/Compilers/Core/Portable/WellKnownTypes.cs Show resolved Hide resolved
}

// InlineArray types must be sequential, as we do arithmetic on them.
var startingOffset = WellKnownType.System_Runtime_CompilerServices_InlineArray2 - 2;
Copy link
Contributor

Choose a reason for hiding this comment

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

var startingOffset = WellKnownType.System_Runtime_CompilerServices_InlineArray2 - 2;

I think all this code is meant for DEBUG build and should be moved into #if DEBUG below.

Copy link
Member Author

Choose a reason for hiding this comment

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

This entire method is Conditional(Debug).

return typeSymbol;
}

internal static bool TryGetWellKnownType(CSharpCompilation compilation, WellKnownType type, BindingDiagnosticBag diagnostics, Location location, [NotNullWhen(true)] out NamedTypeSymbol? typeSymbol)
Copy link
Contributor

Choose a reason for hiding this comment

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

TryGetWellKnownType

I think we should make it clear that this helper is to lookup optional types, for other cases we usually report errors when the type is bad or missing. Consider renaming to "TryGetOptionalWellKnownType"

return false;
}

ReportUseSite(typeSymbol, diagnostics, location);
Copy link
Contributor

Choose a reason for hiding this comment

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

ReportUseSite(typeSymbol, diagnostics, location);

The original suggestion was to mimic behavior of ``internal static Symbol GetWellKnownTypeMember(CSharpCompilation compilation, WellKnownMember member, out UseSiteInfo useSiteInfo, bool isOptional = false)``` and ignore warnings.

{
typeSymbol = compilation.GetWellKnownType(type);
Debug.Assert((object)typeSymbol != null, "Expect an error type if well-known type isn't found");
if (typeSymbol.GetUseSiteInfo().DiagnosticInfo?.Severity == DiagnosticSeverity.Error)
Copy link
Contributor

Choose a reason for hiding this comment

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

typeSymbol.GetUseSiteInfo()

Consider caching in a local and reusing for this check and for reporting as well.

}
};
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

}

I think it would be good to add a test verifying that dependencies are properly tracked. For example, something like the following:

  1. Create library with a built-in inline array type
  2. Create a compilation referencing that library and using a scenario that uses the type implicitly
    3, Verify that the set of used assembly references includes reference to the library with the inline array type.

@AlekseyTs
Copy link
Contributor

Done with review pass (commit 5)

@jcouv jcouv added the Code Gen Quality Room for improvement in the quality of the compiler's generated code label Oct 10, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area-Compilers Code Gen Quality Room for improvement in the quality of the compiler's generated code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

Morty Proxy This is a proxified and sanitized view of the page, visit original site.