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

Helper tool for nullable annotations #4361

Closed
iSazonov opened this issue Oct 23, 2020 · 14 comments
Closed

Helper tool for nullable annotations #4361

iSazonov opened this issue Oct 23, 2020 · 14 comments

Comments

@iSazonov
Copy link

In PowerShell repository we started the nullable annotation process but stopped because of huge complicity - hundreds of cross-referenced classes.

To continue this process, we need to create and arrange the list of classes into groups so that at the beginning there are the simplest classes, in the next group the classes that use them, etc.

Group 0
type name namespace file path

To get such a report, we need a tool based on the Roslyn Analyzer API.

Since this tool would be useful to a large number of projects, not just PowerShell, I post this request here. The tool could be in SDK or be a standalone tool.

@ghost
Copy link

ghost commented Oct 23, 2020

Tagging subscribers to this area: @safern, @ViktorHofer
See info in area-owners.md if you want to be subscribed.

@stephentoub stephentoub transferred this issue from dotnet/runtime Oct 23, 2020
@jaredpar jaredpar transferred this issue from dotnet/roslyn Oct 23, 2020
@iSazonov
Copy link
Author

@mavasani @Evangelink @Youssef1313 Guys, sorry for direct ping but maybe some of you will be interested in this?

@Evangelink
Copy link
Member

So the idea is to provide a tool that would list all types (enum, class, interface, record, struct) with their namespace and their containing file. Could you confirm I got things right?

@Youssef1313
Copy link
Member

@Evangelink What I understood is the tool would need to group classes into levels from the easiest to annotate, to the harder ones.

The level of easiness or hardness will depend on the dependency tree between classes.

@sharwell
Copy link
Member

sharwell commented Nov 27, 2020

To continue this process, we need to create and arrange the list of classes into groups so that at the beginning there are the simplest classes, in the next group the classes that use them, etc.

I'm not sure this is the best way to implement this tool. It tends to lead to frequent cases where annotations are initially incorrect, but then later need to be corrected based on new information. The approach would be less likely to experience this if it went in the other direction:

  • Start by annotating files that do not define any signatures that are referenced by other files
  • Then annotate the files that do not define any signatures that are referenced by remaining unannotated files

But there are also exceptions to this, where you might want to annotate things in other orders:

  • Some files are trivial to annotate because they have no signatures with reference types
  • Some files are easy to annotate without considering other uses by explicitly reviewing the behavior of the implementation and identifying the pre- and post-conditions

My main concern here is a high likelihood that a tool could be implemented, only to find that at the end of the day it's still exactly the same difficulty as it was without the tool.

@iSazonov
Copy link
Author

Oh, I didn't expect you to show interest in my request :-), although I suppose many developers would be happy to find such a tool in .Net SDK.

I'm not sure this is the best way to implement this tool.

Sorry for the bad description! I meant that on top there should be declared types without dependencies on other types declared in the project. The next group contains declared types, which depend only on the types from the previous group. You seem to be talking about the same.

But there are also exceptions to this, where you might want to annotate things in other orders:

Yes, the purpose of this tool is not to create a strategy but to assist in planning. With a report from the tool, developers can choose a path that is best for them.

My main concern here is a high likelihood that a tool could be implemented, only to find that at the end of the day it's still exactly the same difficulty as it was without the tool.

No, no :-) PowerShell Engine (System,Managemet,Automation,csproj) has 734 files and 2422 declared types.
Even a very experienced developer will spend many hours and days analyzing such a volume manually. And of course he/she will think about how to facilitate this work. I would say that even a bad tool would be useful in this situation. :-)

This is exactly what I did.
PowerShell tracking issue PowerShell/PowerShell#12631
You can find there attached Excel file with report (the tool creates csv and I converted it to Excel manually).

The tool is in https://github.com/iSazonov/TypeDependencyHelper
It is more alpha but under MIT and I hope you could modify it to be a tool suitable for .Net SDK.

So the idea is to provide a tool that would list all types (enum, class, interface, record, struct) with their namespace and their containing file. Could you confirm I got things right?

Enums has not references and should be excluded from the report. I added a reference to an issue with attached Excel example.

@sharwell
Copy link
Member

PowerShell Engine (System,Managemet,Automation,csproj) has 734 files and 2422 declared types.

This is much smaller than dotnet/roslyn. My experience there suggests the order of annotations doesn't really matter. You could map out the types based on dependencies, or you could just work in a random order. Both approaches seem to take the same amount of time and yield the same final result.

@iSazonov
Copy link
Author

You could map out the types based on dependencies, or you could just work in a random order. Both approaches seem to take the same amount of time and yield the same final result.

Why then did .Net team create this dotnet/runtime#2339
and now also dotnet/runtime#41720?
Why can't other projects do the same? I wouldn't be surprised if Stephan did it from his memory, but most likely he used some document or tool. And mainly to attract help from the community.

In any case, as PowerShell maintainer, I would rather offer contributors a plan than say - grab any piece of code and annotate it.
Most likely none of them will do anything at all without this, but if they see that it is possible to pick up a part of the code that is interesting and understandable to them, then they will process it.

@Youssef1313
Copy link
Member

Youssef1313 commented Nov 28, 2020

Why then did .Net team create this dotnet/runtime#2339
and now also dotnet/runtime#41720?

Probably the runtime team did the annotation based on what types/assemblies they think are used a lot by developers.
Types that aren't frequently used or legacy were given a much lower priority to annotate. But that's just a guess of what may have happened.

(Note that the runtime provide public APIs that're used by other developers, so annotating makes a different to consumers, so priority based on API popularity sounds very reasonable. This might not be the same for PowerShell if it doesn't provide public APIs to be used by other developers).

@iSazonov
Copy link
Author

iSazonov commented Nov 29, 2020

Probably the runtime team did the annotation based on what types/assemblies they think are used a lot by developers.
Types that aren't frequently used or legacy were given a much lower priority to annotate. But that's just a guess of what may have happened.

From dotnet/runtime#2339

Our desired approach to annotating an assembly is to only do so once all of its dependencies are annotated.

Perhaps there are developers who come to annotate only their public interfaces and do not benefit from annotating their entire project, but this looks like consumer deception. Such a strategy is not even mentioned in the documentation
https://docs.microsoft.com/en-us/dotnet/csharp/nullable-migration-strategies#choose-a-strategy-for-nullable-reference-types

@mavasani
Copy link
Contributor

mavasani commented Feb 2, 2021

@sharwell Can you please drive this issue forward?

@Evangelink
Copy link
Member

Ping @sharwell

@sharwell
Copy link
Member

We are looking at dotnet/roslyn#52893 as the next action item related to nullable reference types.

@Evangelink
Copy link
Member

Do you think there is a need to keep this ticket open or shall it be somehow merged into the other one? Or maybe moved to roslyn side?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants