Ensure <Nullable>enable<Nullable> on System.IO.Packaging and S.Resources.Extensions #41731

krwq · 2020-09-02T08:03:24Z

Looking at the XLS and conversation seems like this is all that's left to do there

…ces.Extensions

Dotnet-GitSync-Bot · 2020-09-02T08:03:27Z

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

safern · 2020-09-02T13:01:16Z

@krwq these are the only projects we we're missing to enable nullable that we're annotated. However the spreadsheet has some other differences in between ref and src. Are you going to tackle those as well?

krwq · 2020-09-02T13:03:23Z

@safern I couldn't see any specific issues on the spreadsheet, the projects there didn't seem to have src annotated (except for perhaps few files from shared directory)

buyaa-n · 2020-09-03T03:29:15Z

Did not updated diff in namespaces

System.ComponentModel.Annotations by assuming we didn't annotated the src completely for a reason (not sure who annotated that, need to clarify)
System.Data.Common same reason, not sure if we want to make the src and ref same
System.ComponentModel.EventBasedAsync cannot even find this ref and src, which project are they?
System.Data.OleDb Also src missing some annotation, might be on purpose
System.IO.Packaging @safern said it annotated correctly, we missed to add enable on the ref
System.Resources.Extensions @safern said it annotated correctly, we missed to add enable on the ref

few diffs were not about nullable annotation, the difference between parameter name or accessor

...aries/System.Diagnostics.DiagnosticSource/ref/System.Diagnostics.DiagnosticSourceActivity.cs

...raries/System.Security.Cryptography.Pkcs/ref/System.Security.Cryptography.Pkcs.netcoreapp.cs

src/libraries/System.Text.Json/ref/System.Text.Json.cs

safern · 2020-09-03T03:47:28Z

System.ComponentModel.Annotations by assuming we didn't annotated the src completely for a reason (not sure who annotated that, need to clarify)
System.Data.Common same reason, not sure if we want to make the src and ref same

@roji do you recall why?

System.ComponentModel.EventBasedAsync cannot even find this ref and src, which project are they?

https://github.com/dotnet/runtime/tree/master/src/libraries/System.ComponentModel.EventBasedAsync/src

Thanks, for working on this, @buyaa-n

buyaa-n · 2020-09-03T03:57:57Z

System.ComponentModel.EventBasedAsync cannot even find this ref and src, which project are they?

https://github.com/dotnet/runtime/tree/master/src/libraries/System.ComponentModel.EventBasedAsync/sr

I checked that, it doesn't have BeginInvoke(object? sender, ProgressChangedEventArgs! e, AsyncCallback callback, object object); and Invoke(object? sender, ProgressChangedEventArgs! e); methods, maybe that is generated for delegates, then i will just fix delegates signature

safern · 2020-09-03T04:05:55Z

Yeah the problem is: public delegate void ProgressChangedEventHandler(object! sender, ProgressChangedEventArgs! e)

buyaa-n · 2020-09-03T05:44:03Z

Fixed System.Data.Common diffs, for some of them source looks correct, but for some ref looks correct, so updated as whichever feels correct to me, please review. Now only 2 ns left, System.ComponentModel.Annotations and System.Data.OleDb, these are not within group 9 => by my understanding, we didn't intend to annotate their src completely so the diff might be OK CC @stephentoub,

stephentoub · 2020-09-03T09:31:26Z

src/libraries/System.Data.Common/ref/System.Data.Common.cs

@@ -863,7 +863,7 @@ public sealed partial class DataTableReader : System.Data.Common.DbDataReader
        public override System.Type GetProviderSpecificFieldType(int ordinal) { throw null; }
        public override object GetProviderSpecificValue(int ordinal) { throw null; }
        public override int GetProviderSpecificValues(object[] values) { throw null; }
-        public override System.Data.DataTable? GetSchemaTable() { throw null; }
+        public override System.Data.DataTable GetSchemaTable() { throw null; }


This looks wrong:
#41082
cc: @roji

I'll revert this

Sorry for the delayed response and thanks - yeah, this is wrong. GetSchemaTable should return nullable as in #41082.

This was changed back per #41731 (comment)

src/libraries/System.Data.Common/ref/System.Data.Common.cs

...braries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/ActivityTagsCollection.cs

jeffhandley · 2020-09-03T22:59:28Z

Once we have this finished up and merged into master, we'll submit it for Tactics Ask Mode approval for RC2:

Use the port bot to port this to release/5.0-rc2 (with a comment of /backport to release/5.0-rc2)
Fill out the PR template in the PR created by the bot
Label it with Servicing-consider
Then I'll send an email to Tactics to request approval
I suspect we'll talk about it during the Tactics meeting on Tuesday

tarekgh · 2020-09-03T23:39:25Z

Thanks @buyaa-n. the changes in System.Diagnostic.DiagnosticsSource looks good.

github-actions · 2020-09-03T23:39:38Z

Started backporting to release/5.0-rc2: https://github.com/dotnet/runtime/actions/runs/238588095

safern · 2020-09-03T23:48:48Z

Applying: ActivityTagsCollection should implement IDictionary<string, object?>
/usr/bin/git push --force --set-upstream origin HEAD:backport/pr-41731-to-release/5.0-rc2
To https://github.com/dotnet/runtime
 + cc835beafa9...31405fa7873 HEAD -> backport/pr-41731-to-release/5.0-rc2 (forced update)
Branch 'backport/pr-41731-to-release/5.0-rc2' set up to track remote branch 'backport/pr-41731-to-release/5.0-rc2' from 'origin'.
Backport temp branch already exists, skipping opening a PR.

We need to re-open the PR.

@akoeplinger re-opening the PRs if the PR is not merged.

safern · 2020-09-03T23:50:45Z

I couldn't re-open the PR. So I deleted the branch and will run the backport again.

/backport to release/5.0-rc2

github-actions · 2020-09-03T23:51:53Z

Started backporting to release/5.0-rc2: https://github.com/dotnet/runtime/actions/runs/238597367

krwq · 2020-09-04T07:01:54Z

Changes from @buyaa-n look good to me

roji · 2020-09-04T08:09:16Z

System.ComponentModel.Annotations by assuming we didn't annotated the src completely for a reason (not sure who annotated that, need to clarify)
System.Data.Common same reason, not sure if we want to make the src and ref same

@buyaa-n @safern yeah, I'm the one who annotated System.ComponentModel.Annotations (#39611), and also System.Data.{Odbc,OleDb} (#39597). Unless I've missed something both that and System.Data.Common should be fully annotated (src and ref) apart from dependencies on TypeConverter and XML. IIRC in at least some of these (especially Odbc/OleDb) I couldn't get errors/warnings for discrepancies between src and ref, so there may have been mismatches...

Are there outstanding issues with these projects (sorry, am a bit late to the party..)? If so I can take a look - feel free to ping me etc.

buyaa-n · 2020-09-04T17:17:30Z

Unless I've missed something both that and System.Data.Common should be fully annotated (src and ref) apart from dependencies on TypeConverter and XML. IIRC in at least some of these (especially Odbc/OleDb) I couldn't get errors/warnings for discrepancies between src and ref, so there may have been mismatches...

@roji There is no error, we have run a tool to find diffs between ref and src and found some diff for those and other assemblies which we are trying to fix with this PR

Are there outstanding issues with these projects (sorry, am a bit late to the party..)? If so I can take a look - feel free to ping me etc.

There is only one issue related to this where the list of findings attached #41696. But no worries i believe we have covered all diffs with this PR, now its need a review

buyaa-n · 2020-09-04T17:26:40Z

This must-have covered/fixed all diffs now (i will try to run @terrajobst's tool again), please review the fixes @safern @stephentoub @terrajobst

...aries/System.Diagnostics.DiagnosticSource/ref/System.Diagnostics.DiagnosticSourceActivity.cs

buyaa-n · 2020-09-05T01:20:11Z

Sorry 😥, it was not covered everything updated left over diffs in:

System.Data.OleDb was not covered
System.IO.Packaging - after enabling nullable most of the diff gone but still there were some diff left

diff in System.Resources.Extensions were fixed after nullable enabled. Now it is covered all. Please review

jeffhandley

LGTM but please wait for other reviews.

src/libraries/System.Data.OleDb/src/OleDbTransaction.cs

safern

LGTM, thanks!

src/libraries/System.Data.Common/src/System/Data/DataTableReader.cs

jeffhandley · 2020-09-08T23:01:08Z

Updating the backport PR to capture latest commits...

/backport to release/5.0-rc2

github-actions · 2020-09-08T23:02:12Z

Started backporting to release/5.0-rc2: https://github.com/dotnet/runtime/actions/runs/245365329

jeffhandley · 2020-09-09T00:45:51Z

Updating the backport PR to capture latest commit...

/backport to release/5.0-rc2

github-actions · 2020-09-09T00:46:54Z

Started backporting to release/5.0-rc2: https://github.com/dotnet/runtime/actions/runs/245476791

jeffhandley · 2020-09-09T18:59:26Z

One last backport to reflect this PR being merged.

/backport to release/5.0-rc2

github-actions · 2020-09-09T19:00:29Z

Started backporting to release/5.0-rc2: https://github.com/dotnet/runtime/actions/runs/246826142

Ensure <Nullable>enable<Nullable> on System.IO.Packaging and S.Resour…

c26f6d1

…ces.Extensions

krwq requested a review from a team September 2, 2020 08:03

stephentoub approved these changes Sep 2, 2020

View reviewed changes

safern approved these changes Sep 2, 2020

View reviewed changes

Fix some of the ref src diff

7c5e566

krwq requested review from jozkee, layomia and steveharter as code owners September 3, 2020 03:12

buyaa-n reviewed Sep 3, 2020

View reviewed changes

...aries/System.Diagnostics.DiagnosticSource/ref/System.Diagnostics.DiagnosticSourceActivity.cs Outdated Show resolved Hide resolved

buyaa-n reviewed Sep 3, 2020

View reviewed changes

...raries/System.Security.Cryptography.Pkcs/ref/System.Security.Cryptography.Pkcs.netcoreapp.cs Show resolved Hide resolved

buyaa-n reviewed Sep 3, 2020

View reviewed changes

src/libraries/System.Text.Json/ref/System.Text.Json.cs Show resolved Hide resolved

buyaa-n added 2 commits September 2, 2020 21:25

delegate sender nullable, fix build failures

056e1c5

Fix diff in System.Data.Common

8dccffd

Fix build failure

06ce738

buyaa-n force-pushed the fix-diff-impl-ref branch from 9b52171 to 06ce738 Compare September 3, 2020 06:00

stephentoub reviewed Sep 3, 2020

View reviewed changes

src/libraries/System.Data.Common/ref/System.Data.Common.cs Outdated Show resolved Hide resolved

krwq added 3 commits September 3, 2020 11:43

fix couple more impl vs ref diffs

1f06f94

Apply feedback

6be56b1

DisallowNull on ActivityTagsCollection [] operator

8b8bb97

tarekgh reviewed Sep 3, 2020

View reviewed changes

...braries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/ActivityTagsCollection.cs Outdated Show resolved Hide resolved

github-actions bot mentioned this pull request Sep 3, 2020

[release/5.0-rc2] Fixes to nullable reference type annotations #41845

Merged

jeffhandley reviewed Sep 5, 2020

View reviewed changes

...aries/System.Diagnostics.DiagnosticSource/ref/System.Diagnostics.DiagnosticSourceActivity.cs Outdated Show resolved Hide resolved

jeffhandley approved these changes Sep 5, 2020

View reviewed changes

buyaa-n reviewed Sep 5, 2020

View reviewed changes

src/libraries/System.Data.OleDb/src/OleDbTransaction.cs Show resolved Hide resolved

safern approved these changes Sep 8, 2020

View reviewed changes

jozkee mentioned this pull request Sep 8, 2020

Nullable annotation mismatch in System.Text.Json.JsonException #41956

Closed

Fix/update left overs

b172655

buyaa-n force-pushed the fix-diff-impl-ref branch from 8d9215b to b172655 Compare September 8, 2020 17:25

roji reviewed Sep 8, 2020

View reviewed changes

src/libraries/System.Data.Common/src/System/Data/DataTableReader.cs Outdated Show resolved Hide resolved

DataTableReader.GetSchemaTable() should return non null

77e699b

buyaa-n approved these changes Sep 9, 2020

View reviewed changes

buyaa-n merged commit 3ac170a into dotnet:master Sep 9, 2020

roji mentioned this pull request Sep 18, 2020

Annotate System.ComponentModel.DataAnnotations for nullability #39611

Merged

ghost locked as resolved and limited conversation to collaborators Dec 7, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ensure <Nullable>enable<Nullable> on System.IO.Packaging and S.Resources.Extensions #41731

Ensure <Nullable>enable<Nullable> on System.IO.Packaging and S.Resources.Extensions #41731

krwq commented Sep 2, 2020

Dotnet-GitSync-Bot commented Sep 2, 2020

safern commented Sep 2, 2020

krwq commented Sep 2, 2020

buyaa-n commented Sep 3, 2020 •

edited

Loading

safern commented Sep 3, 2020

buyaa-n commented Sep 3, 2020

safern commented Sep 3, 2020

buyaa-n commented Sep 3, 2020

stephentoub Sep 3, 2020

krwq Sep 3, 2020

roji Sep 4, 2020 •

edited

Loading

jeffhandley Sep 9, 2020

jeffhandley commented Sep 3, 2020

tarekgh commented Sep 3, 2020

github-actions bot commented Sep 3, 2020

safern commented Sep 3, 2020

safern commented Sep 3, 2020

github-actions bot commented Sep 3, 2020

krwq commented Sep 4, 2020 •

edited

Loading

roji commented Sep 4, 2020

buyaa-n commented Sep 4, 2020 •

edited

Loading

buyaa-n commented Sep 4, 2020

buyaa-n commented Sep 5, 2020

jeffhandley left a comment

safern left a comment

jeffhandley commented Sep 8, 2020

github-actions bot commented Sep 8, 2020

jeffhandley commented Sep 9, 2020

github-actions bot commented Sep 9, 2020

jeffhandley commented Sep 9, 2020

github-actions bot commented Sep 9, 2020

Ensure <Nullable>enable<Nullable> on System.IO.Packaging and S.Resources.Extensions #41731

Ensure <Nullable>enable<Nullable> on System.IO.Packaging and S.Resources.Extensions #41731

Conversation

krwq commented Sep 2, 2020

Dotnet-GitSync-Bot commented Sep 2, 2020

safern commented Sep 2, 2020

krwq commented Sep 2, 2020

buyaa-n commented Sep 3, 2020 • edited Loading

safern commented Sep 3, 2020

buyaa-n commented Sep 3, 2020

safern commented Sep 3, 2020

buyaa-n commented Sep 3, 2020

stephentoub Sep 3, 2020

Choose a reason for hiding this comment

krwq Sep 3, 2020

Choose a reason for hiding this comment

roji Sep 4, 2020 • edited Loading

Choose a reason for hiding this comment

jeffhandley Sep 9, 2020

Choose a reason for hiding this comment

jeffhandley commented Sep 3, 2020

tarekgh commented Sep 3, 2020

github-actions bot commented Sep 3, 2020

safern commented Sep 3, 2020

safern commented Sep 3, 2020

github-actions bot commented Sep 3, 2020

krwq commented Sep 4, 2020 • edited Loading

roji commented Sep 4, 2020

buyaa-n commented Sep 4, 2020 • edited Loading

buyaa-n commented Sep 4, 2020

buyaa-n commented Sep 5, 2020

jeffhandley left a comment

Choose a reason for hiding this comment

safern left a comment

Choose a reason for hiding this comment

jeffhandley commented Sep 8, 2020

github-actions bot commented Sep 8, 2020

jeffhandley commented Sep 9, 2020

github-actions bot commented Sep 9, 2020

jeffhandley commented Sep 9, 2020

github-actions bot commented Sep 9, 2020

buyaa-n commented Sep 3, 2020 •

edited

Loading

roji Sep 4, 2020 •

edited

Loading

krwq commented Sep 4, 2020 •

edited

Loading

buyaa-n commented Sep 4, 2020 •

edited

Loading