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

Make GetDeclaredSymbol more strongly typed for LocalFunctionDeclarationSyntax #71034

Merged
merged 1 commit into from
Nov 30, 2023

Conversation

CyrusNajmabadi
Copy link
Member

No description provided.

@CyrusNajmabadi CyrusNajmabadi requested a review from a team as a code owner November 30, 2023 18:24
@dotnet-issue-labeler dotnet-issue-labeler bot added Area-Compilers untriaged Issues and PRs which have not yet been triaged by a lead labels Nov 30, 2023
@CyrusNajmabadi
Copy link
Member Author

@333fred @jcouv @RikkiGibson trivial type safety PR.

Copy link
Contributor

@RikkiGibson RikkiGibson left a comment

Choose a reason for hiding this comment

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

lgtm, seems to make things consistent with the CompilationUnitSyntax overload below.

@CyrusNajmabadi
Copy link
Member Author

CyrusNajmabadi commented Nov 30, 2023

@RikkiGibson Yup. These should always be strongly typed. The only times they shouldn't be is if you can genuinely get different symbol types back. For example, an IPropertySymbol and an IMethodSymbol (which share no common base interface except ISymbol) :)

@@ -2960,7 +2960,7 @@ internal Conversion ClassifyConversionForCast(int position, ExpressionSyntax exp
/// <param name="declarationSyntax">The syntax node that declares a member.</param>
/// <param name="cancellationToken">The cancellation token.</param>
/// <returns>The symbol that was declared.</returns>
public abstract ISymbol GetDeclaredSymbol(LocalFunctionStatementSyntax declarationSyntax, CancellationToken cancellationToken = default(CancellationToken));
public abstract IMethodSymbol GetDeclaredSymbol(LocalFunctionStatementSyntax declarationSyntax, CancellationToken cancellationToken = default(CancellationToken));
Copy link
Member

Choose a reason for hiding this comment

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

Huh. I was going to suggest that we should update the public API for this as well, but we apparently don't have a GetDeclaredSymbol extension for local functions. That seems like a bug in itself, as it means that dotnet/roslyn-analyzers#6779 is going to warn when calling GetDeclaredSymbol on a local function syntax node.

Copy link
Member

Choose a reason for hiding this comment

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

And now I see that you're driving this because of a request for just such an API in #71028 😆.

Copy link
Member Author

Choose a reason for hiding this comment

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

yup :D

@CyrusNajmabadi CyrusNajmabadi merged commit b8dc316 into dotnet:main Nov 30, 2023
25 checks passed
@ghost ghost added this to the Next milestone Nov 30, 2023
@CyrusNajmabadi CyrusNajmabadi deleted the strongerTypes branch November 30, 2023 20:35
@Cosifne Cosifne modified the milestones: Next, 17.9 P3 Jan 9, 2024
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.

None yet

4 participants