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

Sunday cleanup project: replace internal usage analyzer with [Experimental] #33385

Closed
wants to merge 1 commit into from

Conversation

roji
Copy link
Member

@roji roji commented Mar 24, 2024

Some notes (I can bring this to a design discussion):

  • [EntityFrameworkInternal] is replaced directly with [Experimental], easy.
  • For the Internal namespace check, I've added [Experimental] to all types, and added an API consistency check that verifies (via reflection) that any exported types from the assembly have the attribute.
  • Note that we want to suppress warnings on usage of these "experimental" APIs within the project/assembly, but have the warnings when the APIs are referenced from the outside (like internal). This means we need multiple diagnostic codes, to e.g. allow EFCore to suppress usage of its own APIs internally, but to have warnings reported when EF.Relational uses them.
  • I've added diagnostic IDs for EFCore, EFCore.Relational, EFCore.Design, EFCore.Proxies, and another one covering all providers.

Possibly future directions:

  • Remove the Internal namespaces altogether, now that we have an [Experimental] attribute everywhere. The main blocker is external tools such as API docs generation, which I think currently filter out internal APIs by namespace (maybe attribute-based filtering is possible too).
  • Remove the "this is an internal API" comment, as the attribute is now bullet-proof, present everywhere and hopefully conveys the message.

Closes #33384

@roji roji requested a review from a team March 24, 2024 13:46
@ajcvickers
Copy link
Member

Note: I am very strongly against replacing our specific pubternal attribute, which is for one thing, with Experimental, which is for another thing.

@@ -231,10 +231,12 @@ public static DbCommand CreateDbCommand(this IQueryable source)
public static IQueryable<TEntity> AsSingleQuery<TEntity>(
this IQueryable<TEntity> source)
where TEntity : class
#pragma warning disable EF9901
Copy link
Member Author

Choose a reason for hiding this comment

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

This is an example of a scenario missed by the analyzer (is checks)

@@ -1937,12 +1939,14 @@ protected override Expression VisitConditional(ConditionalExpression conditional
// jsonManagerPrm.CaptureState();
for (var i = 0; i < 5; i++)
{
#pragma warning disable EF9901
Copy link
Member Author

Choose a reason for hiding this comment

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

Another example of a case missed by the analyzer - typeof

@roji
Copy link
Member Author

roji commented Mar 24, 2024

I am very strongly against replacing our specific pubternal attribute, which is for one thing, with Experimental, which is for another thing.

I must have misunderstood - I thought we raised this possibility and you were in favor.

I'll bring this to design.

@ajcvickers
Copy link
Member

I must have misunderstood - I thought we raised this possibility and you were in favor.

I'll bring this to design.

I'm in favor of using Experimental when we have API that is intended to be used by applications, but needs time to bake. (We occasionally make such API pubternal today.) But I believe that we need to be as explicit and in-your-face as possible with the pubternal messaging, since it's an unusual aspect of our code base that I think it's important people don't accidentally miss.

@roji
Copy link
Member Author

roji commented Mar 24, 2024

Just to provide context, someone using an API annotated with [Experimental] gets the following compilation warning:

Error EF1001 : 'XXX' is for evaluation purposes only and is subject to change or removal in future updates. Suppress this diagnostic to proceed. (http://aka.ms/....)

At the end of the day, the user doesn't really get exposed to the name of the attribute - it could be called anything; the important thing here IMHO is that we have a standard, reliable mechanism for generating a compilation warning that doesn't involve us maintaing our own analyzer. The message seems close enough to me, with the important part again being "subject to change or removal in future updates". The user is very explicitly getting an in-your-face message.

If we believe the semantic difference in the un-exposed name ("experimental" vs. "internal") is important enough to keep maintaining our own analyzer mechanism, well, that's not going to be a hill II'm going to die on...

(Note also that the diagnostic ID is specified as part of the attribute, and a URL can point the user at specific documentation as well, clarifying the situation etc.)

@maumar
Copy link
Contributor

maumar commented Mar 25, 2024

can we hold off with this one at least until #33351 is checked in?

@roji
Copy link
Member Author

roji commented Mar 25, 2024

Sure

@roji
Copy link
Member Author

roji commented Mar 25, 2024

Closing as per design decision 😢

@roji roji closed this Mar 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Replace our internal usage analyzer with .NET 8.0 [Experimental]
3 participants