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.

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

Ensure errors are reported when compiler is unable to emit a required attribute. #70556

Merged
merged 2 commits into from
Oct 26, 2023

Conversation

AlekseyTs
Copy link
Contributor

Related to #70338.

@AlekseyTs AlekseyTs requested a review from a team as a code owner October 25, 2023 18:52
@dotnet-issue-labeler dotnet-issue-labeler bot added the untriaged Issues and PRs which have not yet been triaged by a lead label Oct 25, 2023
@AlekseyTs
Copy link
Contributor Author

@dotnet/roslyn-compiler For the second review.

@@ -48,7 +48,7 @@ internal abstract partial class EmbeddedTypesManager<
where TModuleCompilationState : CommonModuleCompilationState
where TEmbeddedTypesManager : EmbeddedTypesManager<TPEModuleBuilder, TModuleCompilationState, TEmbeddedTypesManager, TSyntaxNode, TAttributeData, TSymbol, TAssemblySymbol, TNamedTypeSymbol, TFieldSymbol, TMethodSymbol, TEventSymbol, TPropertySymbol, TParameterSymbol, TTypeParameterSymbol, TEmbeddedType, TEmbeddedField, TEmbeddedMethod, TEmbeddedEvent, TEmbeddedProperty, TEmbeddedParameter, TEmbeddedTypeParameter>
where TSyntaxNode : SyntaxNode
where TAttributeData : AttributeData, Cci.ICustomAttribute
where TAttributeData : class, Cci.ICustomAttribute
Copy link
Member

@cston cston Oct 26, 2023

Choose a reason for hiding this comment

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

class

Was there a reason for this change? #Resolved

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Was there a reason for this change?

Yes, I want to prevent the common component to take advantage of the APIs available from AttributeData.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Among other things, this ensures that I adjusted all "interesting" code without creating error tests for all affected attributes.

return attributeConstructor.GetUseSiteInfo().DiagnosticInfo;

case { }:
return new CSDiagnosticInfo(ErrorCode.ERR_BogusType, string.Empty);
Copy link
Member

@cston cston Oct 26, 2023

Choose a reason for hiding this comment

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

string.Empty

AttributeClass? #Resolved

Copy link
Contributor Author

Choose a reason for hiding this comment

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

AttributeClass?

No. There is nothing wrong with the attribute class here. We simply failed to crack the attribute application, i.e. dealing with some sort of invalid metadata for the specific attribute application. In such cases elsewhere we create UnsupportedMetadataTypeSymbol and this is how we report a use-site error for it.


If AttributeConstructor IsNot Nothing Then
Return If(AttributeConstructor.GetUseSiteInfo().DiagnosticInfo,
ErrorFactory.ErrorInfo(ERRID.ERR_UnsupportedType1, String.Empty))
Copy link
Member

@cston cston Oct 26, 2023

Choose a reason for hiding this comment

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

String.Empty

AttributeClass? #Resolved

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same response as for C#

@AlekseyTs AlekseyTs merged commit 06072fb into dotnet:main Oct 26, 2023
25 checks passed
@ghost ghost added this to the Next milestone Oct 26, 2023
@jjonescz jjonescz modified the milestones: Next, 17.9 P1 Oct 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Compilers untriaged Issues and PRs which have not yet been triaged by a lead
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants