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

Display standalone parameter names by default #67857

Merged

Conversation

jjonescz
Copy link
Contributor

@jjonescz jjonescz commented Apr 18, 2023

Closes #67478.
Closes #67632.
Closes #67464.
Motivated by #67478 (comment).

Changes symbol display formatter to include names if used on IParameterSymbol directly by default (unless compiler-internal option is specified).

@dotnet-issue-labeler dotnet-issue-labeler bot added Area-Compilers untriaged Issues and PRs which have not yet been triaged by a lead labels Apr 18, 2023
@jjonescz jjonescz force-pushed the 67464-StandaloneParameterName-ByDefault branch from 237c7a9 to 5e7ae18 Compare April 18, 2023 09:31
@jjonescz jjonescz closed this Apr 18, 2023
@jjonescz jjonescz reopened this Apr 18, 2023
@jjonescz jjonescz marked this pull request as ready for review April 18, 2023 12:09
@jjonescz jjonescz requested review from a team as code owners April 18, 2023 12:09
@jcouv jcouv self-assigned this Apr 18, 2023
@jcouv jcouv removed the untriaged Issues and PRs which have not yet been triaged by a lead label Apr 18, 2023
@jcouv jcouv added this to the 17.7 milestone Apr 18, 2023
Copy link
Member

@jcouv jcouv left a comment

Choose a reason for hiding this comment

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

LGTM Thanks (iteration 4)

@jjonescz
Copy link
Contributor Author

@dotnet/roslyn-compiler for a second review
@CyrusNajmabadi FYI

@jcouv
Copy link
Member

jcouv commented May 3, 2023

@dotnet/roslyn-compiler for a second review

`SymbolDisplayFormat.CSharpErrorMessageFormat` and `CSharpShortErrorMessageFormat` now include parameter names by default if used on a standalone `IParameterSymbol`.
For example, parameter `p` in `void M(ref int p)` was previously formatted as `"ref int"` and now it is formatted as `"ref int p"`.

# Version 4.6.0
Copy link
Contributor

@AlekseyTs AlekseyTs May 4, 2023

Choose a reason for hiding this comment

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

What is this version for? #Closed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For Roslyn NuGet package, although I guess this change will land in 4.7.0.

Suggested change
# Version 4.6.0
# Version 4.7.0

@AlekseyTs
Copy link
Contributor

Did we get an official approval for the breaking change? Otherwise, commit 5 looks good, but I am not signing off yet.

# Version 4.5.0

`SymbolDisplayFormat.CSharpErrorMessageFormat` and `CSharpShortErrorMessageFormat` now include parameter names by default if used on a standalone `IParameterSymbol`.
For example, parameter `p` in `void M(ref int p)` was previously formatted as `"ref int"` and now it is formatted as `"ref int p"`.
Copy link
Member

Choose a reason for hiding this comment

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

FYI, we called these 'Display' apis intentionally so that we coudl change them without it being considered a breaking change. I'm fine documenting this though, just wanted to reiterate our position that things that exist for 'display purposes' absolutely can and will change depending on what we think is best for displaying them :)


### `SymbolDisplayFormat` includes parameter name when invoked on `IParameterSymbol`

All `SymbolDisplayFormat`s (predefined and user-created) now include parameter names by default if used on a standalone `IParameterSymbol` for consistency with predefined formats (see the breaking change for version 4.5.0 above).

### Changed `IncrementalStepRunReason` when a modified input produced a new output
Copy link
Contributor

@AlekseyTs AlekseyTs May 12, 2023

Choose a reason for hiding this comment

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

It looks like this section is completely unrelated to the goal of this PR #Closed

Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like this section is completely unrelated to the goal of this PR

I guess it came from the parent branch with the merge.

Copy link
Contributor Author

@jjonescz jjonescz May 12, 2023

Choose a reason for hiding this comment

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

Yes, this PR hasn't touched that section.

@AlekseyTs
Copy link
Contributor

Done with review pass (commit 7)

Copy link
Contributor

@AlekseyTs AlekseyTs left a comment

Choose a reason for hiding this comment

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

LGTM (commit 7)

@jjonescz jjonescz merged commit 1497e87 into dotnet:main May 12, 2023
28 checks passed
@jjonescz jjonescz deleted the 67464-StandaloneParameterName-ByDefault branch May 12, 2023 13:26
@ghost ghost removed this from the 17.7 milestone May 12, 2023
@ghost ghost added this to the Next milestone May 12, 2023
@Cosifne Cosifne modified the milestones: Next, 17.7 P2 May 31, 2023
Zoxive referenced this pull request in Zoxive/MemoizeSourceGenerator Sep 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment