Ensure <Nullable>enable<Nullable> on System.IO.Packaging and S.Resources.Extensions by krwq · Pull Request #41731 · dotnet/runtime

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

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

 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)

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

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.

safern

LGTM, thanks!

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 teamSeptember 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

Comment thread...aries/System.Diagnostics.DiagnosticSource/ref/System.Diagnostics.DiagnosticSourceActivity.cs Outdated

buyaa-n reviewed Sep 3, 2020
View reviewed changes

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

buyaa-n reviewed Sep 3, 2020
View reviewed changes

Comment threadsrc/libraries/System.Text.Json/ref/System.Text.Json.cs

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 06ce738Compare September 3, 2020 06:00

stephentoub reviewed Sep 3, 2020
View reviewed changes

Comment threadsrc/libraries/System.Data.Common/ref/System.Data.Common.cs

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

Comment thread...braries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/ActivityTagsCollection.cs Outdated

github-actionsBot 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

Comment thread...aries/System.Diagnostics.DiagnosticSource/ref/System.Diagnostics.DiagnosticSourceActivity.cs Outdated

jeffhandley approved these changes Sep 5, 2020
View reviewed changes

buyaa-n reviewed Sep 5, 2020
View reviewed changes

Comment threadsrc/libraries/System.Data.OleDb/src/OleDbTransaction.cs Outdated

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 b172655Compare September 8, 2020 17:25

roji reviewed Sep 8, 2020
View reviewed changes

Comment threadsrc/libraries/System.Data.Common/src/System/Data/DataTableReader.cs Outdated

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:masterSep 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

Uh oh!

Conversation

krwq commented Sep 2, 2020

Uh oh!

Dotnet-GitSync-Bot commented Sep 2, 2020

Uh oh!

safern commented Sep 2, 2020

Uh oh!

krwq commented Sep 2, 2020

Uh oh!

buyaa-n commented Sep 3, 2020• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

safern commented Sep 3, 2020

Uh oh!

buyaa-n commented Sep 3, 2020

Uh oh!

safern commented Sep 3, 2020

Uh oh!

buyaa-n commented Sep 3, 2020

Uh oh!

stephentoubSep 3, 2020

Choose a reason for hiding this comment

Uh oh!

krwqSep 3, 2020

Choose a reason for hiding this comment

Uh oh!

rojiSep 4, 2020• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jeffhandleySep 9, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jeffhandley commented Sep 3, 2020

Uh oh!

tarekgh commented Sep 3, 2020

Uh oh!

github-actionsBot commented Sep 3, 2020

Uh oh!

safern commented Sep 3, 2020

Uh oh!

safern commented Sep 3, 2020

Uh oh!

github-actionsBot commented Sep 3, 2020

Uh oh!

krwq commented Sep 4, 2020• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

roji commented Sep 4, 2020

Uh oh!

buyaa-n commented Sep 4, 2020• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

buyaa-n commented Sep 4, 2020

Uh oh!

Uh oh!

buyaa-n commented Sep 5, 2020

Uh oh!

jeffhandley left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

safern left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jeffhandley commented Sep 8, 2020

Uh oh!

github-actionsBot commented Sep 8, 2020

Uh oh!

jeffhandley commented Sep 9, 2020

Uh oh!

github-actionsBot commented Sep 9, 2020

Uh oh!

buyaa-n commented Sep 3, 2020•
edited
Loading

rojiSep 4, 2020•
edited
Loading

krwq commented Sep 4, 2020•
edited
Loading

buyaa-n commented Sep 4, 2020•
edited
Loading