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

RouteConstraints aren't linker friendly #24723

Closed
davidfowl opened this issue Aug 9, 2020 · 6 comments · Fixed by #38014
Closed

RouteConstraints aren't linker friendly #24723

davidfowl opened this issue Aug 9, 2020 · 6 comments · Fixed by #38014
Labels
affected-few This issue impacts only small number of customers api-approved API was approved in API review, it can be implemented area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates bug This issue describes a behavior which is not expected - a bug. feature-routing linker-friendliness Tracking linker friendliness severity-major This label is used by an internal tool

Comments

@davidfowl
Copy link
Member

davidfowl commented Aug 9, 2020

Today RouteConstraints are described by a RouteOptions.ConstraintMap. The issue with this approach is the linker doesn't have way to know that these types are activated elsewhere in the system. We need an API that lets us describe adding a constraint that is type safe and lets us describe that we want to preserve constructors:

public class RouteOptions
{
    public void AddParameterPolicy<[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicConstructors)]T>(string text) where T : IParameterPolicy;
    public void AddParameterPolicy(string text, [DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicConstructors)]Type constraintType);
}

This would let the linker know that these constraint types should be kept around.

Here's the current warning:

C:\dev\git\aspnetcore\src\Http\Routing\src\ParameterPolicyActivator.cs(93,13): Trim analysis warning IL2006: Microsoft.AspNetCore.Routing.ParameterPolicyActivator.CreateParameterPolicy(IServiceProvider,Type,String): The parameter 'parameterPolicyType' of method 'Microsoft.AspNetCore.Routing.ParameterPolicyActivator.CreateParameterPolicy(IServiceProvider,Type,String)' with dynamically accessed member kinds 'None' is passed into the implicit 'this' parameter of method 'System.Type.GetConstructors()' which requires dynamically accessed member kinds 'PublicConstructors'. To fix this add DynamicallyAccessedMembersAttribute to it and specify at least these member kinds 'PublicConstructors'. [C:\Users\davifowl\source\repos\WebApplication60\WebApplication60\WebApplication60.csproj]
@davidfowl davidfowl added the linker-friendliness Tracking linker friendliness label Aug 9, 2020
@davidfowl davidfowl added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Aug 9, 2020
@Pilchie Pilchie added area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates feature-routing labels Aug 10, 2020
@pranavkm pranavkm added this to the Backlog milestone Aug 11, 2020
@ghost
Copy link

ghost commented Aug 11, 2020

We've moved this issue to the Backlog milestone. This means that it is not going to be worked on for the coming release. We will reassess the backlog following the current release and consider this item at that time. To learn more about our issue management process and to have better expectation regarding different types of issues you can read our Triage Process.

@SteveSandersonMS SteveSandersonMS added affected-most This issue impacts most of the customers enhancement This issue represents an ask for new feature or an enhancement to an existing one severity-minor This label is used by an internal tool labels Oct 7, 2020 — with ASP.NET Core Issue Ranking
@SteveSandersonMS SteveSandersonMS added severity-major This label is used by an internal tool affected-few This issue impacts only small number of customers bug This issue describes a behavior which is not expected - a bug. and removed severity-minor This label is used by an internal tool affected-most This issue impacts most of the customers enhancement This issue represents an ask for new feature or an enhancement to an existing one labels Oct 7, 2020
@BrennanConroy BrennanConroy added area-servers and removed area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates labels Mar 30, 2021
@ghost
Copy link

ghost commented Apr 20, 2021

Thanks for contacting us.
We're moving this issue to the Next sprint planning milestone for future evaluation / consideration. Because it's not immediately obvious that this is a bug in our framework, we would like to keep this around to collect more feedback, which can later help us determine the impact of it. We will re-evaluate this issue, during our next planning meeting(s).
If we later determine, that the issue has no community involvement, or it's very rare and low-impact issue, we will close it - so that the team can focus on more important and high impact issues.
To learn more about what to expect next and how this issue will be handled you can read more about our triage process here.

@javiercn javiercn added the area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates label May 6, 2021
@mkArtakMSFT mkArtakMSFT removed the area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates label Aug 12, 2021
@mkArtakMSFT
Copy link
Member

@davidfowl we don't plan to handle this in 6.0 If you neither, feel free to move to backlog.

@mkArtakMSFT mkArtakMSFT added the area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates label Aug 12, 2021
@ghost
Copy link

ghost commented Aug 12, 2021

We've moved this issue to the Backlog milestone. This means that it is not going to be worked on for the coming release. We will reassess the backlog following the current release and consider this item at that time. To learn more about our issue management process and to have better expectation regarding different types of issues you can read our Triage Process.

@rafikiassumani-msft rafikiassumani-msft added the api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews label Oct 26, 2021
@ghost
Copy link

ghost commented Oct 26, 2021

Thank you for submitting this for API review. This will be reviewed by @dotnet/aspnet-api-review at the next meeting of the ASP.NET Core API Review group. Please ensure you take a look at the API review process documentation and ensure that:

  • The PR contains changes to the reference-assembly that describe the API change. Or, you have included a snippet of reference-assembly-style code that illustrates the API change.
  • The PR describes the impact to users, both positive (useful new APIs) and negative (breaking changes).
  • Someone is assigned to "champion" this change in the meeting, and they understand the impact and design of the change.

@rafikiassumani-msft rafikiassumani-msft removed api-suggestion Early API idea and discussion, it is NOT ready for implementation area-web-frameworks labels Oct 26, 2021
@pranavkm
Copy link
Contributor

pranavkm commented Nov 1, 2021

API review:

Approved API:

public class RouteOptions
{
+    public void SetParameterPolicy<[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicConstructors)]T>(string text) where T : IParameterPolicy;
+    public void SetParameterPolicy(string text, [DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicConstructors)]Type constraintType);
}

The API is approved. We we want to prevent users from mutating Routeoptions.ConstraintMap since that could again introduce trimmer unfriendly behavior. We think the best way to do this is to annotate the property with RequiresUnreferencedCodeAttribute and make a note that says the warning can be suppressed if all they are doing is reading or deleting from the dictionary.

The API is named Setxxx since it will always replace an existing key.

@pranavkm pranavkm added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews labels Nov 1, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Dec 2, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
affected-few This issue impacts only small number of customers api-approved API was approved in API review, it can be implemented area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates bug This issue describes a behavior which is not expected - a bug. feature-routing linker-friendliness Tracking linker friendliness severity-major This label is used by an internal tool
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants