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.

Sign up for GitHub

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

Jump to bottom

Move Editor Attribute to S.CM.Primitives and apply to types that had it in netfx #41145

Merged
merged 4 commits into from
Aug 23, 2020
Merged

Move Editor Attribute to S.CM.Primitives and apply to types that had it in netfx #41145

merged 4 commits into from
Aug 23, 2020

Conversation

safern
Copy link
Member

@safern safern commented Aug 21, 2020

Fixes: #31428

Also, I fixed a couple of type full names on DesignerSerializableAttributes that were missing Culture=neutral.

cc: @RussKie

@safern safern requested review from eerhardt and ericstj August 21, 2020 08:41
@Dotnet-GitSync-Bot
Copy link
Collaborator

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

@Dotnet-GitSync-Bot
Copy link
Collaborator

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.

ericstj
ericstj reviewed Aug 21, 2020
View reviewed changes
src/libraries/System.ComponentModel.TypeConverter/ref/System.ComponentModel.TypeConverter.cs
@@ -1563,6 +1551,7 @@ public abstract partial class DesignerOptionService : System.ComponentModel.Desi
protected virtual bool ShowDialog(System.ComponentModel.Design.DesignerOptionService.DesignerOptionCollection options, object optionObject) { throw null; }
object System.ComponentModel.Design.IDesignerOptionService.GetOptionValue(string pageName, string valueName) { throw null; }
void System.ComponentModel.Design.IDesignerOptionService.SetOptionValue(string pageName, string valueName, object value) { }
[System.ComponentModel.EditorAttribute("", "System.Drawing.Design.UITypeEditor, System.Drawing, Version=4.0.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a")]
Copy link
Member

Choose a reason for hiding this comment

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

Empty string looks odd but that seems consistent with .NETFramework

Copy link
Member Author

Choose a reason for hiding this comment

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

I guess if we wanted we could have UITypeEditor as the editor type and base type empty?

src/libraries/System.Data.Odbc/ref/System.Data.Odbc.cs Outdated Show resolved Hide resolved
eerhardt
eerhardt reviewed Aug 21, 2020
View reviewed changes
src/libraries/System.Data.OleDb/ref/System.Data.OleDb.cs Outdated
System.Data.IDataReader System.Data.IDbCommand.ExecuteReader() { throw null; }
System.Data.IDataReader System.Data.IDbCommand.ExecuteReader(System.Data.CommandBehavior behavior) { throw null; }
object System.ICloneable.Clone() { throw null; }
System.Data.IDataReader? System.Data.IDbCommand.ExecuteReader() { throw null; }
Copy link
Member

Choose a reason for hiding this comment

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

Was this just a missed auto-gen the ref code from a previous commit?

Copy link
Member Author

Choose a reason for hiding this comment

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

Right.

Copy link
Member

Choose a reason for hiding this comment

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

Something is wrong here.

runtime/src/libraries/System.Data.Common/src/System/Data/IDbCommand.cs

Lines 22 to 23 in 89292fe

IDataReader ExecuteReader();
IDataReader ExecuteReader(CommandBehavior behavior);

Copy link
Member

Choose a reason for hiding this comment

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

runtime/src/libraries/System.Data.OleDb/src/OleDbCommand.cs

Lines 605 to 608 in 89292fe

IDataReader IDbCommand.ExecuteReader()
{
return ExecuteReader(CommandBehavior.Default);
}

runtime/src/libraries/System.Data.OleDb/src/OleDbCommand.cs

Lines 616 to 619 in 89292fe

IDataReader IDbCommand.ExecuteReader(CommandBehavior behavior)
{
return ExecuteReader(behavior);
}

None of these are marked as nullable.

Copy link
Member Author

Choose a reason for hiding this comment

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

I guess GenAPI has a bug where it is finding a NullableContextAttribute(2) in the same context given that these are non-visible members and we tell Roslyn to not preserve Nullable* metadata for non-visible members. Maybe we should just skip these members in GenAPI or just match the interface annotation instead.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed.

eerhardt
eerhardt reviewed Aug 21, 2020
View reviewed changes
src/libraries/System.Data.OleDb/ref/System.Data.OleDb.cs Outdated
object System.ICloneable.Clone() { throw null; }
System.Data.IDataReader? System.Data.IDbCommand.ExecuteReader() { throw null; }
System.Data.IDataReader? System.Data.IDbCommand.ExecuteReader(System.Data.CommandBehavior behavior) { throw null; }
object? System.ICloneable.Clone() { throw null; }
Copy link
Member

Choose a reason for hiding this comment

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

Even the Clone() method appears to be wrong:

runtime/src/libraries/System.Private.CoreLib/src/System/ICloneable.cs

Lines 6 to 9 in aa5fdab

public interface ICloneable
{
object Clone();
}

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed.

eerhardt
eerhardt approved these changes Aug 21, 2020
View reviewed changes
Copy link
Member

@eerhardt eerhardt left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks!

@safern safern mentioned this pull request Aug 21, 2020
Open
@safern
Copy link
Member Author

safern commented Aug 21, 2020 

C:\h\w\A41C090F\w\B544097D\e>"C:\h\w\A41C090F\p\dotnet.exe" exec --runtimeconfig Common.Tests.runtimeconfig.json --depsfile Common.Tests.deps.json xunit.console.dll Common.Tests.dll -xml testResults.xml -nologo -nocolor -notrait category=IgnoreForCI -notrait category=OuterLoop -notrait category=failing  
Failed to load the dll from [C:\h\w\A41C090F\p\shared\Microsoft.NETCore.App\6.0.0\coreclr.dll], HRESULT: 0x8007007F
Failed to bind to CoreCLR at 'C:\h\w\A41C090F\p\shared\Microsoft.NETCore.App\6.0.0\'
Failed to create CoreCLR, HRESULT: 0x80008088

@jkotas I've never seen this before. I downloaded the payload and I do see coreclr.dll in the path it looked for it.

@safern
Copy link
Member Author

safern commented Aug 23, 2020

Failures are: #41215

@safern safern merged commit 3130ac9 into dotnet:master Aug 23, 2020
@safern safern deleted the EditorAttribute branch August 23, 2020 00:24
safern added a commit to safern/runtime that referenced this pull request Aug 24, 2020
@safern
Move Editor Attribute to S.CM.Primitives and apply to types that had …
9ae4126 
…it in netfx (dotnet#41145)

* Move EditorAttribute to System.ComponentModel.Primitives

* Add EditorAttribute to types that had it in netfx and fix some DesignerSerializableAttributes

* PR Feedback

* PR Feedback
@safern safern mentioned this pull request Aug 24, 2020
Merged
safern added a commit that referenced this pull request Aug 25, 2020
@safern
Move Editor Attribute to S.CM.Primitives and apply to types that had …
c8d0f74 
…it in netfx (#41145) (#41289)

* Move EditorAttribute to System.ComponentModel.Primitives

* Add EditorAttribute to types that had it in netfx and fix some DesignerSerializableAttributes

* PR Feedback

* PR Feedback
@ghost ghost locked as resolved and limited conversation to collaborators Dec 7, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@ericstj ericstj ericstj approved these changes

@eerhardt eerhardt eerhardt approved these changes

Assignees
No one assigned
Labels
area-Meta
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

IComponent is missing designer attributes
5 participants
@safern @Dotnet-GitSync-Bot @eerhardt @ericstj @danmoseley