-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
[wasm] Mark System.Console APIs as unsupported on Browser #41184
Conversation
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. |
Tagging subscribers to this area: @safern, @ViktorHofer |
b958789
to
091167b
Compare
[System.Runtime.Versioning.SupportedOSPlatformAttribute("windows")] | ||
public static bool NumberLock { get { throw null; } } | ||
public static System.IO.TextWriter Out { get { throw null; } } | ||
public static System.Text.Encoding OutputEncoding { get { throw null; } set { } } | ||
[System.Runtime.Versioning.UnsupportedOSPlatformAttribute("browser")] | ||
public static string Title { [System.Runtime.Versioning.SupportedOSPlatformAttribute("windows")] get { throw null; } set { } } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure is this would be considered an "inconsistent list." Should [System.Runtime.Versioning.UnsupportedOSPlatformAttribute("browser")]
instead be applied right before set
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just posted a similar question on the src
file before seeing this comment. #41184 (comment)
[UnsupportedOSPlatform("browser")] | ||
public static bool CursorVisible | ||
{ | ||
[SupportedOSPlatform("windows")] | ||
get { return ConsolePal.CursorVisible; } | ||
set { ConsolePal.CursorVisible = value; } | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is an interesting scenario! @buyaa-n can you weigh in on how this will be interpreted with the property being marked as [Unsupported("browser")]
but the get
method having [SupportedOSPlatform("windows")]
? Is this OK or should we instead mark the set
method as [UnsupportedOSPlatform("browser")]
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the UnsupportedOSPlatform
even necessary? I imagine that the behavior of the analyzer would be "if there is a SupportedOSPlatformAttribute
" it will flag the API as unsupported on any other platforms?
Maybe we should just mark the whole property as SupportedOSPlatform("windows")
? It seems weird to be able to set a property that you can't get on the same platform? Or does this property affects other APIs that are supported?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What was intended here was to mark the property getter as supported only on Windows, but the property setter as supported everywhere (except browser now). The getter and setter can have their own attributes. But I don't know what the analyzer will do when the getter/setter differ from the property itself like this.
It might be clearer to just mark the set
method as [UnsupportedOSPlatform("browser")]
even if this would actually work as-is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see. Maybe something to consider on the Analyzer? That if the property has an attribute, but the getter/setter has another, the getter/setter wins?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is an interesting scenario! @buyaa-n can you weigh in on how this will be interpreted with the property being marked as [Unsupported("browser")] but the get method having [SupportedOSPlatform("windows")]? Is this OK or should we instead mark the set method as [UnsupportedOSPlatform("browser")]?
Yes it would be considered as an inconsistent list, as you mentioned the [UnsupportedOSPlatform("browser")] attribute better to be in the setter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see. Maybe something to consider on the Analyzer? That if the property has an attribute, but the getter/setter has another, the getter/setter wins?
It works that way in case they are for the same platform, but not for different platform
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool. @mdh1418 per these comments, please update all properties where there was already [SupportedOSPlatform("windows")]
on one of the accessors but not the other, and move the [UnsupportedOSPlatform("browser")]
attribute down to the other accessor instead of on the property itself.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
move UnsupportedOSPlatform attributes down to the accessor
Sorry, I probably wasn't clear enough in my previous comment. We should only move attributes down to the accessors when there are existing attributes on the other accessor.
For properties only with get
, the attribute can be on the property itself. And for properties with both get
and set
that did not previously have [SupportedOSPlatform("windows")]
you should also apply the new attribute to the entire property (assuming both accessors are unsupported on browser).
We only need to move the attribute to the accessors when support between the 2 accessors differs.
31abd9c
to
30783d8
Compare
Ah, right, so the attribute will be applied to the highest level. I removed a commit that had previously moved the attributes down to the accessor level. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!! This is exciting to see.
Hello @safern! Because this pull request has the p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (
|
* [wasm] Mark System.Console APIs as unsupported on Browser * System.Console remove unsupported attribute from Console.Clear() * System.Console mark SetIn as unsupported on Browser * System.Console move UnsupportedOSPlatform attributes down to the accessor Co-authored-by: Mitchell Hwang <mitchell.hwang@microsoft.com>
* [wasm] Mark System.ComponentModel APIs as unsupported on Browser (#41094) * [wasm] System.ComponentModel enable platform attributes * [wasm] Mark ExtendedProtectionPolicyTypeConverter.ConvertTo as unsupported * [wasm] Mark System.ComponentModel.TypeDescriptor.CreateInstance as unsupported * [wasm] Mark System.ComponentModel.TypeDescriptionProvider.CreateInstance as unsupported * [wasm] Mark System.ComponentModel.LicenseManager.CreateWithContext as unsupported * [wasm] Mark System.ComponentModel.MaskedTextProvider.Clone as unsupported Co-authored-by: Mitchell Hwang <mitchell.hwang@microsoft.com> * [wasm] Mark System.Console APIs as unsupported on Browser (#41184) * [wasm] Mark System.Console APIs as unsupported on Browser * System.Console remove unsupported attribute from Console.Clear() * System.Console mark SetIn as unsupported on Browser * System.Console move UnsupportedOSPlatform attributes down to the accessor Co-authored-by: Mitchell Hwang <mitchell.hwang@microsoft.com> * Mark System.Diagnostics.FileVersionInfo as unsupported on Browser (#41271) Co-authored-by: Mitchell Hwang <mitchell.hwang@microsoft.com> * Mark System.Diagnostics.Process unsupported at assembly level (#41694) Co-authored-by: Mitchell Hwang <mitchell.hwang@microsoft.com> * [wasm] Mark System.IO.Compression APIs as unsupported on Browser (#41683) * [wasm] System.IO.Compression.Brotli enable platform attributes * Mark System.IO.Compression.Brotli unsupported at assembly level Co-authored-by: Mitchell Hwang <mitchell.hwang@microsoft.com> * [wasm] Mark System.IO.FileSystem.Watcher APIs as unsupported on Browser (#41682) * [wasm] System.IO.FileSystem.Watcher enable platform attributes * Mark System.IO.FileSystem.Watcher unsupported at assembly level Co-authored-by: Mitchell Hwang <mitchell.hwang@microsoft.com> * [wasm] Mark System.IO.IsolatedStorage APIs as unsupported on Browser (#41700) * [wasm] System.IO.IsolatedStorage enable platform attributes * Mark System.IO.IsolatedStorage Unsupported at assembly level Co-authored-by: Mitchell Hwang <mitchell.hwang@microsoft.com> * Mark some System.Net.* APIs as unsupported on Browser WASM (#40924) * [wasm] Mark System.Threading.ThreadPool unsupported on Browser (#41891) * System.Threading.ThreadPool enable platform attributes * Mark System.Threading.ThreadPool APIs unsupported on browser * System.Threading.ThreadPool Add Unsupported attribute to other ThreadPool files * Remove Unsupported attributes from BindHandle * Add windows Supported Attribute to BindHandle Co-authored-by: Mitchell Hwang <mitchell.hwang@microsoft.com> * Mark System.Net.WebSockets.ClientWebSocketOptions APIs as unsupported on Browser (#41963) * Mark System.Net.WebSockets.ClientWebSocketOptions APIs as unsupported on Browser * Add the attributes to non-browser version of ClientWebSocketOptions class to avoid build error * Add using * Include platform attributes * [wasm] Mark System.Net.NameResolution Unsupported at assembly level (#41985) Co-authored-by: Mitchell Hwang <mitchell.hwang@microsoft.com> Co-authored-by: Mitchell Hwang <mitchell.hwang@microsoft.com> Co-authored-by: Maxim Lipnin <v-maxlip@microsoft.com>
* [wasm] Mark System.Console APIs as unsupported on Browser * System.Console remove unsupported attribute from Console.Clear() * System.Console mark SetIn as unsupported on Browser * System.Console move UnsupportedOSPlatform attributes down to the accessor Co-authored-by: Mitchell Hwang <mitchell.hwang@microsoft.com>
Contributes to #41087