Update visibility of component parameters

[rynowak]

Summary

We have to make a final decision about the visibility of component parameters. We've run into many issues trying to use non-public properties, and I don't think these are going to stop.

Problems and Goals

• We want to make it hard or impossible to set a component parameter property directly. This is never correct when done in application code
• We've tried using non-public properties and found a bunch of things that don't work
  • Reference docs require public
  • Reference assemblies require public
  • Our metadata analysis has (possibly fragile) workarounds in place that we use to look at non-public properties
• We want whatever we choose to work with AOT, but we don't exactly know the requirements. It's generally assumed in AOT that unused non-public code is dead code and can be removed.
• We want whatever we choose to work with testing, but we don't know the requirements of that yet

TLDR we have given non-public properties a try and the results aren't great. We got here because we now have to maintain ref assembly code for our components manually. It would be great if the guidance for users worked for us as well.

Some options

We could recommend that users use public get properties, and we will call the setter regardless of its visibility.

We could recommend that users use public get/set properties and we will write an analyzer to tell you not to set component parameter properties.

I don't like the option that we'd keep on doing analysis over non-public properties. We don't do this anywhere else I can think of, and I don't have confidence that this will play out in our favor wrt tooling.

Conclusion

We have a conclusion here!

Preview 8

• Reference the component analyzer package from .Components
• Add analyzer to warn if a parameter is set in code outside of the inheritance hierarchy or a nested class
• Update our samples
• Verify [CascadingParameter]s do not get Analyzesr warnings impacted
• Don't pass analyzers to the declaration compilation target
• Add our component analyzer package to the SDK
• Add a .targets file to our analyzer package to avoid duplicates when included by both the SDK and package
• Make parameters public get/set
• Add analyzer to warn if a property is not public get/set and has [Parameter]
• Update our tests

Before final release

• Remove workarounds in TagHelper discovery - Tracked by Remove TagHelper discovery private property metadata workaround #12291
• Change from warning to error in the analyzer - Tracked by Change [Parameter] warning (BL0004) to be an error - Non-public parameters #12294

We can leave [CascadingParameter] properties alone. They don't interact with tooling, don't want documentation, and don't represent a public contract.

[egil]

I am not sure if this is a valid scenario, but moving to virtual protected/virtual public parameters might make component inheritance easier. I.e. one could make a specialized version of the NavLink component by inheriting from LavLink instead of wrapping it.

[rynowak]

Right you are. We have examples of this in the framework like InputText that are designed for inheritance.

[vertonghenb]

Isn't there a guideline that we should consider using the "Composition over inheritance strategy" when using components? I think it's also used in React etc.

[SteveSandersonMS]

We have to make a final decision about the visibility of component parameters. We've run into many issues trying to use non-public properties, and I don't think these are going to stop.

Overall, very much agreed. C# doesn't have a modifier that really matches the scenario perfectly.

We could recommend that users use public get/set properties and we will write an analyzer to tell you not to set component parameter properties.

Sounds like the best option if such an analyzer is feasible. I don't know the perf characteristics of analyzers in the IDE, but of course this means our analyzer has to inspect every property-set call in every method in every opened project while you're typing. If it scales to that then great.

We could still allow developers to have private set on their own components if they want (either way, the framework is using reflection to set, so it doesn't matter to us). We would have public set on framework components to avoid all the tooling/docs/etc issues. I don't feel strongly about this though.

We want whatever we choose to work with AOT, but we don't exactly know the requirements. It's generally assumed in AOT that unused non-public code is dead code and can be removed.

It's also generally assumed that unused public code is dead code that can be removed, given that AOT operates on applications not on libraries. So I don't expect changing the visibility is going to make anything better for us here.

[rynowak]

Sounds like the best option if such an analyzer is feasible. I don't know the perf characteristics of analyzers in the IDE, but of course this means our analyzer has to inspect every property-set call in every method in every opened project while you're typing. If it scales to that then great.

This kind of thing actually works really well. You can to inspect every property when running a full build. When you're editing, you get to look at the class being worked on. The analyzer infrastructure just does this for you, it's pretty neat.

[rynowak]

The only concern that I have here is that we have people different guidance than we have been - but I think it's worth changing what we're doing, and in that case we can't avoid telling people to change their code.

IMO we would want to roll this out in preview 7 along with an analyzer that tells you to conform to our guidlelines. Then in preview 8 (or later) we remove supports for non-public properties.

[vertonghenb]

@rynowak I don't mind the breaking change, it's for the better and we're still in preview, so break it! 😄

[xclud]

I vote for the breaking change.

[rynowak]

@pranavkm @SteveSandersonMS - please give this a look over and make sure it all makes sense.

[NTaylorMullen]

All required items for this have been completed.