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

Nullable annotate ListViewGroup #2828

Merged
merged 3 commits into from May 6, 2020
Merged

Conversation

hughbe
Copy link
Contributor

@hughbe hughbe commented Feb 9, 2020

Proposed Changes

  • Nullable annotate ListViewGroup
  • Fix NRE bug
Microsoft Reviewers: Open in CodeFlow

@hughbe hughbe requested a review from a team as a code owner February 9, 2020 13:34
@msftbot msftbot bot assigned hughbe Feb 9, 2020
@codecov
Copy link

codecov bot commented Feb 9, 2020

Codecov Report

Merging #2828 into master will increase coverage by 0.00307%.
The diff coverage is n/a.

@@                 Coverage Diff                 @@
##              master       #2828         +/-   ##
===================================================
+ Coverage   59.43311%   59.43618%   +0.00307%     
===================================================
  Files           1236        1236                 
  Lines         429535      429538          +3     
  Branches       38780       38781          +1     
===================================================
+ Hits          255286      255301         +15     
+ Misses        168887      168879          -8     
+ Partials        5362        5358          -4     
Flag Coverage Δ
#Debug 59.43618% <100.00000%> (+0.00307%) ⬆️
#production 32.14102% <100.00000%> (+0.00553%) ⬆️
#test 98.96944% <100.00000%> (ø) ⬆️

@RussKie RussKie added ⛔ blocked Blocked by external dependencies area: NRT labels Feb 11, 2020
JuditRose
JuditRose previously approved these changes Feb 24, 2020
Copy link
Contributor

@JuditRose JuditRose left a comment

Choose a reason for hiding this comment

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

LGTM

@RussKie
Copy link
Member

RussKie commented Mar 16, 2020

Just a heads up: we're currently blocked by our downstream consumers and unable to update to the latest .NET Runtime, which contains the latest compiler needed for the Roslyn analysers.
It maybe another month or so... I'll provide an update when the situation changes.

@hughbe hughbe force-pushed the ListViewGroup-nullable branch 4 times, most recently from 9a881d7 to f1c90d0 Compare April 21, 2020 10:01
@RussKie RussKie removed the ⛔ blocked Blocked by external dependencies label Apr 30, 2020
@RussKie
Copy link
Member

RussKie commented Apr 30, 2020

You need to apply PubllicAPI analyzer fixes to record the changes to the public API surface.

@RussKie RussKie added the 📭 waiting-author-feedback The team requires more information from the author label Apr 30, 2020
@msftbot msftbot bot removed the 📭 waiting-author-feedback The team requires more information from the author label Apr 30, 2020
@hughbe
Copy link
Contributor Author

hughbe commented Apr 30, 2020

Right... So I think I've figured out how to do this.

For my sanity:

  • "~" before each line does something: what it does, god knows. I have a hunch it disables nullable annotations? I cannot find anything annotated for the life of me
  • You need to explicitly annotate non-null parameters with ! at the end

There's one more things that I think is a bug in the analyzer. There is an error

System/Windows/Forms/ListViewGroup.cs(168,16): warning RS0036: Symbol 'Items.get' is missing nullability annotations in the declared API. [/Users/hugh/Documents/GitHub/winforms/src/System.Windows.Forms/src/System.Windows.Forms.csproj]

ListViewGroup.Items is defined as non-nullable in c#

public ListView.ListViewItemCollection Items
    => _items ??= new ListView.ListViewItemCollection(new ListViewGroupItemCollection(this));

and in the `PublicApi.Shipped.txt":

System.Windows.Forms.ListViewGroup.Items.get -> System.Windows.Forms.ListView.ListViewItemCollection!

So I'm confused why this error occurs

Please can there be some sort of option to just regenerate PublicApi.Shipped.txt. To be honest, there might be but there doesn't seem to be any docs :))

@hughbe
Copy link
Contributor Author

hughbe commented May 1, 2020

/cc @jcouv in case this is a bug

@hughbe
Copy link
Contributor Author

hughbe commented May 1, 2020

Yup. This is very obviously a bug. See the latest commit for the fix. I am guessing the analyzer doesn't understand ??= syntax

@jcouv
Copy link
Member

jcouv commented May 1, 2020

@hughbe @RussKie

"~" before each line does something

This marker just documents that this API still mentions an oblivious type. The purpose is to help track annotation effort (ie. remove all those markers).
I'm adding some documentation as I see the kinds of questions you have. Let me know if still needs improvement. Thanks for your patience :-)

You need to explicitly annotate non-null parameters with ! at the end

Yes, when the publicAPI file includes #nullable enable, then ! means non-nullable. This contrasts with "" (blank) which would mean oblivious. And ? means nullable.

error related to System.Windows.Forms.ListViewGroup.Items.get

I'm confused too. Just to confirm, was the line in publicAPI file (System.Windows.Forms.ListViewGroup.Items.get -> System.Windows.Forms.ListView.ListViewItemCollection!) generated by publicAPI analyzer/fixer, or did you edit it by hand (I would recommend against that in general).

Let's sync on Monday. We can do LiveShare or Teams meeting to troubleshoot.

See the latest commit for the fix. I am guessing the analyzer doesn't understand ??= syntax

If you mean the publicAPI analyzer, then I don't think so. That analyzer only looks at public APIs, not the bodies/implementations of methods.

@jcouv
Copy link
Member

jcouv commented May 1, 2020

Please can there be some sort of option to just regenerate PublicApi.Shipped.txt.

Depending on the warning, a fix should be offered at the location of the warning. Select the desired fix (with Project or Solution scope) and that should be it.

@jcouv jcouv unassigned hughbe May 1, 2020
@msftbot msftbot bot assigned hughbe May 1, 2020
@jcouv jcouv self-assigned this May 1, 2020
@RussKie
Copy link
Member

RussKie commented May 4, 2020

Let's sync on Monday. We can do LiveShare or Teams meeting to troubleshoot.

I don't think it is possible, PST, GMT and AEST don't overlap... I'd like to sync up about dotnet/roslyn-analyzers#3582

@hughbe
Copy link
Contributor Author

hughbe commented May 4, 2020

Yeah you're right. I've updated ListViewGroup.Header/Footer to have AllowNull attributes (meaning that you can set null but it never returns null`) and made the header parameters in the constructors nullable

Copy link
Member

@RussKie RussKie left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@RussKie
Copy link
Member

RussKie commented May 4, 2020

The builds is failing complaining about annotations.... Right now NRT annotation feel like a tremendous up hill battle. 😩

@hughbe
Copy link
Contributor Author

hughbe commented May 4, 2020

I know... I may be blind, but I don't get the error

System\Windows\Forms\ListViewGroup.cs#L170
System\Windows\Forms\ListViewGroup.cs(170,57): error RS0036: (NETCORE_ENGINEERING_TELEMETRY=Build) Symbol 'Items.get' is missing nullability annotations in the declared API.

@weltkante
Copy link
Contributor

I played around with this a bit, looks like the tool cannot handle nested classes as return values. Probably needs to be raised as an issue somewhere else.

@hughbe
Copy link
Contributor Author

hughbe commented May 4, 2020

I suspected as much!

@jcouv
Copy link
Member

jcouv commented May 5, 2020

The builds is failing complaining about annotations.... Right now NRT annotation feel like a tremendous up hill battle.

Consider annotating your code without putting #nullable enable in the public API file for now.
I'll look into the nested type issue. I thought that was fixed already.

@jcouv
Copy link
Member

jcouv commented May 5, 2020

Confirmed the bug with return of nested type. Will fix asap

@jcouv
Copy link
Member

jcouv commented May 5, 2020

Hum, seems like I spoke too soon. I cannot repro the issue (found a bug with a compiler API, but that's not the one used by the publicAPI analyzer...). Below is the test to verify that.

I wonder if you may be using a version that predates this fix. I'll take a look at your package versions and get back to you.

        [Fact]
        public async Task AnnotatedMemberInAnnotatedShippedAPI()
        {
            var source = @"
#nullable enable
public class C
{
    public string? OldField;
    public string? {|RS0036:Field|};
    public string {|RS0036:Field2|};
    public Nested? {|RS0036:Field3|};
    public Nested {|RS0036:Field4|};
    public class Nested { }
}
";

            var shippedText = @"#nullable enable
C
C.C() -> void
C.Nested
C.Nested.Nested() -> void
C.OldField -> string?
C.Field -> string
C.Field2 -> string
C.Field3 -> C.Nested
C.Field4 -> C.Nested";

            var unshippedText = @"";

            var fixedShippedText = @"#nullable enable
C
C.C() -> void
C.Nested
C.Nested.Nested() -> void
C.OldField -> string?
C.Field -> string?
C.Field2 -> string!
C.Field3 -> C.Nested?
C.Field4 -> C.Nested!";

            await VerifyCSharpAdditionalFileFixAsync(source, shippedText, unshippedText, fixedShippedText, newUnshippedApiText: unshippedText);
        }

@jcouv
Copy link
Member

jcouv commented May 5, 2020

Alright, I dug into this. It is indeed a problem that was fixed in more recent publicAPI analyzer. I didn't trace the fix down though.
Here's a change that I made locally and built successfully: 049f256

@mavasani I noticed that the most recent publicAPI analyzer on nuget is 3.0. But on myget, we're up to 3.3-beta.
Could we publish a 3.2 or a 3.3-beta so that winforms repo can reference it without adding myget as a source?

@mavasani
Copy link
Member

mavasani commented May 5, 2020

Just published https://www.nuget.org/packages/Microsoft.CodeAnalysis.PublicApiAnalyzers/3.3.0-beta1.final, which should have all the changes to this analyzer.

@jcouv
Copy link
Member

jcouv commented May 5, 2020

Thanks @mavasani :-)

@hughbe I've pushed a commit that updates the roslyn-analyzer package reference and fixes publicAPI file. I hope that's okay.

@RussKie
Copy link
Member

RussKie commented May 5, 2020

Looks like the build agent is unable to find the analyzer:

##[error]src\System.Windows.Forms\src\System.Windows.Forms.csproj(0,0): error NU1603: (NETCORE_ENGINEERING_TELEMETRY=Restore) 
System.Windows.Forms depends on Microsoft.CodeAnalysis.PublicApiAnalyzers (>= 3.3.0-beta1.20255.4) but Microsoft.CodeAnalysis.PublicApiAnalyzers 3.3.0-beta1.20255.4 was not found. An approximate best match of Microsoft.CodeAnalysis.PublicApiAnalyzers 3.3.0-beta1.final was resolved.

@jcouv
Copy link
Member

jcouv commented May 5, 2020

I'd accidentally reverted the version with a wrong checkout operation... Pushed a commit to fix that. Thanks

@RussKie RussKie added the 📖 documentation: breaking A change in behavior that could be breaking for applications. Needs to be documented. label May 6, 2020
@RussKie RussKie merged commit 25b5072 into dotnet:master May 6, 2020
@msftbot msftbot bot added this to the 5.0 Preview5 milestone May 6, 2020
@RussKie
Copy link
Member

RussKie commented May 6, 2020

Thank you!

@RussKie
Copy link
Member

RussKie commented May 6, 2020

/cc: @Logerfo looks like we are starting to make progress

@RussKie
Copy link
Member

RussKie commented May 6, 2020

Docs: dotnet/docs#17085

@hughbe hughbe deleted the ListViewGroup-nullable branch May 6, 2020 08:22
@msftbot msftbot bot locked as resolved and limited conversation to collaborators Feb 2, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
📖 documentation: breaking A change in behavior that could be breaking for applications. Needs to be documented.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants