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

Do not emit unused fields in RDG source #48426

Merged
merged 3 commits into from
Jul 10, 2023
Merged

Do not emit unused fields in RDG source #48426

merged 3 commits into from
Jul 10, 2023

Conversation

martincostello
Copy link
Member

Do not emit unused fields in RDG source

Elide private fields that are not used by emitted RDG code.

Description

Do not emit unused private fields for HTTP verbs that are not used by any of the user-code endpoints.

Fixes #48381.

@ghost ghost added area-runtime community-contribution Indicates that the PR has been added by a community member labels May 25, 2023
@ghost
Copy link

ghost commented May 25, 2023

Thanks for your PR, @martincostello. Someone from the team will get assigned to your PR shortly and we'll get it reviewed.

@martincostello martincostello mentioned this pull request May 25, 2023
Closed
1 task
@martincostello martincostello added feature-rdg area-minimal Includes minimal APIs, endpoint filters, parameter binding, request delegate generator etc and removed area-runtime labels May 29, 2023
@mitchdenny
Copy link
Member

Thanks @martincostello, this looks good and is well covered by our existing test cases. I'm approving, but also tagging @captainsafia just in case there is an issue I've missed.

@captainsafia
Copy link
Member

@martincostello @mitchdenny Apologies for the delay!

I was trying to form a more coherent note of my feedback here.

The gist of my reservations is that it feels rather strange for us to use a completely different strategy for emit for this static fields compared to what we have for the existing helper types and methods. Typically, we'd use the EmitterContext to set state for types/methods that need to be emitted as we render their invocations. It would be worthwhile to explore doing this instead of the mapping out the verbs in EmitVerbs and see if we can have a discrete helperFields pipeline for those fields.

@martincostello
Copy link
Member Author

No problem - I wasn't sure on what the best way was, so went with an initial approach that wouldn't churn the code for the generator too much.

If you can point me at roughly what you're referring to for the context and the pipeline I'll look at changing the approach once #48555 is merged as otherwise I think it'll be a massive merge conflict nightmare.

@ghost
Copy link

ghost commented Jun 9, 2023

Looks like this PR hasn't been active for some time and the codebase could have been changed in the meantime.
To make sure no breaking changes are introduced, please leave an /azp run comment here to rerun the CI pipeline and confirm success before merging the change.

@ghost ghost added the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Jun 9, 2023
@martincostello martincostello force-pushed the issue-48381-only-emit-used-fields branch from d056819 to e4c1c32 Compare June 9, 2023 07:55
@captainsafia captainsafia removed the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Jun 9, 2023
@martincostello martincostello force-pushed the issue-48381-only-emit-used-fields branch from e4c1c32 to 6ad9975 Compare June 12, 2023 13:47
@ghost
Copy link

ghost commented Jun 20, 2023

Looks like this PR hasn't been active for some time and the codebase could have been changed in the meantime.
To make sure no breaking changes are introduced, please leave an /azp run comment here to rerun the CI pipeline and confirm success before merging the change.

@ghost ghost added the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Jun 20, 2023
@mitchdenny
Copy link
Member

@martincostello this is looking good. Looking at the Helix test failures it looks like the baselines just need to be regenerated.

@martincostello martincostello force-pushed the issue-48381-only-emit-used-fields branch from 6ad9975 to 35c7c8d Compare June 20, 2023 10:24
@martincostello martincostello removed the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Jun 20, 2023
@mitchdenny
Copy link
Member

/app run

@ghost
Copy link

ghost commented Jun 28, 2023

Looks like this PR hasn't been active for some time and the codebase could have been changed in the meantime.
To make sure no breaking changes are introduced, please leave an /azp run comment here to rerun the CI pipeline and confirm success before merging the change.

@ghost ghost added the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Jun 28, 2023
@martincostello martincostello force-pushed the issue-48381-only-emit-used-fields branch from 35c7c8d to cb0f236 Compare June 28, 2023 15:03
@martincostello martincostello removed the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Jun 28, 2023
@ghost
Copy link

ghost commented Jul 6, 2023

Looks like this PR hasn't been active for some time and the codebase could have been changed in the meantime.
To make sure no breaking changes are introduced, please leave an /azp run comment here to rerun the CI pipeline and confirm success before merging the change.

@ghost ghost added the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Jul 6, 2023
@martincostello
Do not emit unused fields in RDG source
a136e78 
Do not emit unused private fields for HTTP verbs that are not used by any of the user-code endpoints.
Resolves dotnet#48381.
@martincostello
Move verbs to incremental pipeline
cbf5452 
Use another incremental pipeline to produce the HTTP verbs.
@martincostello
Add custom comparer
24f3b68 
Add custom comparer for endpoints by their HTTP method.
@martincostello martincostello force-pushed the issue-48381-only-emit-used-fields branch from cb0f236 to 24f3b68 Compare July 7, 2023 13:48
@martincostello martincostello removed the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Jul 7, 2023
captainsafia
captainsafia approved these changes Jul 10, 2023
View reviewed changes
Copy link
Member

@captainsafia captainsafia left a comment

Choose a reason for hiding this comment

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

🤩

@captainsafia captainsafia merged commit 427163d into dotnet:main Jul 10, 2023
@ghost ghost added this to the 8.0-preview7 milestone Jul 10, 2023
@martincostello martincostello deleted the issue-48381-only-emit-used-fields branch July 10, 2023 22:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@captainsafia captainsafia captainsafia approved these changes

@mitchdenny mitchdenny mitchdenny approved these changes

@Tratcher Tratcher Awaiting requested review from Tratcher

@BrennanConroy BrennanConroy Awaiting requested review from BrennanConroy BrennanConroy is a code owner

@halter73 halter73 Awaiting requested review from halter73 halter73 is a code owner

Assignees

@adityamandaleeka adityamandaleeka

Labels
area-minimal Includes minimal APIs, endpoint filters, parameter binding, request delegate generator etc community-contribution Indicates that the PR has been added by a community member feature-rdg
Projects
None yet
Milestone
8.0-preview7
Development

Successfully merging this pull request may close these issues.

Do not emit unused code with Request Delegate Generator
4 participants
@martincostello @mitchdenny @captainsafia @adityamandaleeka