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

Use Roslyn interceptors feature in binder gen #90340

Merged
merged 5 commits into from
Aug 19, 2023
Merged

Use Roslyn interceptors feature in binder gen #90340

merged 5 commits into from
Aug 19, 2023

Conversation

layomia
Copy link
Contributor

@layomia layomia commented Aug 10, 2023 •
edited
Loading

Fixes #89322.

@layomia layomia added this to the 8.0.0 milestone Aug 10, 2023
@layomia layomia self-assigned this Aug 10, 2023
@ghost
Copy link

ghost commented Aug 10, 2023

Tagging subscribers to this area: @dotnet/area-extensions-configuration
See info in area-owners.md if you want to be subscribed.

Issue Details

Fixes #89322.

Work left

Author: layomia
Assignees: layomia
Labels:

area-Extensions-Configuration
Milestone: 8.0.0

@build-analysis build-analysis bot mentioned this pull request Aug 11, 2023
Open
@layomia layomia force-pushed the binder-gen-interceptors branch 2 times, most recently from 7e0ce7c to 4c4022c Compare August 15, 2023 22:06
This was referenced Aug 16, 2023
Closed
Closed
Closed
@layomia layomia force-pushed the binder-gen-interceptors branch 2 times, most recently from 88cb774 to d205853 Compare August 17, 2023 15:41
@layomia layomia marked this pull request as ready for review August 17, 2023 16:30
@layomia layomia requested review from eerhardt, eiriktsarpalis, captainsafia and tarekgh and removed request for jeffhandley August 17, 2023 16:30
@ericstj
Copy link
Member

ericstj commented Aug 17, 2023

image

🎉 🎉 🎉 Reviewers - let's have a look at this and try to get feedback in and addressed ASAP to make it for RC1.

ericstj
ericstj reviewed Aug 17, 2023
View reviewed changes
...aries/Microsoft.Extensions.Configuration.Binder/gen/ConfigurationBindingGenerator.Emitter.cs
[AttributeUsage(AttributeTargets.Method, AllowMultiple = true)]
file sealed class InterceptsLocationAttribute : Attribute
{
public InterceptsLocationAttribute(string filePath, int line, int column)
Copy link
Member

Choose a reason for hiding this comment

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

Did we want to add something like https://github.com/dotnet/sdk/blob/4249701de6f005c670c7f371141e772d02f89156/src/WebSdk/ProjectSystem/Targets/Microsoft.NET.Sdk.Web.ProjectSystem.targets#L64 to our NuGet package targets, or will we ask users to set this? @eerhardt @tarekgh @captainsafia

Copy link
Member

@tarekgh tarekgh Aug 17, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I think we decided to mimic whatever the aspnet is doing in the web project.

Copy link
Member

Choose a reason for hiding this comment

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

ASP.NET doesn't ship a nuget package so we're special here. There's an ongoing discussion around this to decide what we want to do.

@layomia layomia force-pushed the binder-gen-interceptors branch 4 times, most recently from 80df566 to 743f89a Compare August 18, 2023 19:34
@layomia layomia closed this Aug 18, 2023
@layomia layomia reopened this Aug 18, 2023
@layomia layomia force-pushed the binder-gen-interceptors branch 2 times, most recently from 4303ea5 to d8aa6d9 Compare August 18, 2023 21:01
@layomia layomia mentioned this pull request Aug 18, 2023
Closed
@layomia
Copy link
Contributor Author

layomia commented Aug 18, 2023

Closing notes

Name clashes

Not needed for this feature to work & there's no new risk added here. I'll address in #86363. I've reverted the changes to the options generator and left out unique name generation in this generator.

Testing

There's always room to improve coverage, but I believe these suffice for merging this PR.

  • Interceptors handling multiple calls, including across different files.
    • Emitted source for the binder test project shows this is well covered.
  • Diagnostic suppressions where applicable
    • numerous binding calls in extensions assemblies which are linker trimmed show that this handled correctly
  • Not suppressing diagnostics for calls we didn't intercept:
    • tested with simple console application
  • Polymorphic type handling: Microsoft.Extensions.Logging.Console assembly exercises/validates the updated logic.

@layomia
Copy link
Contributor Author

layomia commented Aug 18, 2023

/backport to release/8.0-rc1

@github-actions
Copy link
Contributor

Started backporting to release/8.0-rc1: https://github.com/dotnet/runtime/actions/runs/5907490321

@github-actions github-actions bot mentioned this pull request Aug 18, 2023
Merged
@layomia
Copy link
Contributor Author

layomia commented Aug 18, 2023

Test failures seem to be caused by applying the code analysis version update across all projects. Will fix.

@layomia layomia merged commit a5ff66c into dotnet:main Aug 19, 2023
171 of 173 checks passed
@jaredpar jaredpar mentioned this pull request Aug 21, 2023
Merged
This was referenced Aug 22, 2023
Merged
Merged
@MichaelSimons MichaelSimons mentioned this pull request Aug 24, 2023
Merged
@ghost ghost locked as resolved and limited conversation to collaborators Sep 18, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@tmat tmat tmat left review comments

@stephentoub stephentoub stephentoub left review comments

@eiriktsarpalis eiriktsarpalis eiriktsarpalis approved these changes

@RikkiGibson RikkiGibson RikkiGibson left review comments

@eerhardt eerhardt eerhardt left review comments

@ericstj ericstj ericstj left review comments

@tarekgh tarekgh tarekgh approved these changes

@captainsafia captainsafia Awaiting requested review from captainsafia

Assignees

@layomia layomia

Labels
area-Extensions-Configuration
Projects
None yet
Milestone
8.0.0
Development

Successfully merging this pull request may close these issues.

Use Roslyn interceptors feature in config binder generator
9 participants
@layomia @ericstj @tmat @captainsafia @stephentoub @eiriktsarpalis @RikkiGibson @eerhardt @tarekgh