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

[generator] Do not generate PlatformNotSupportedException in chaining .ctor #7085

Merged
merged 2 commits into from
Sep 24, 2019

Conversation

spouliot
Copy link
Contributor

Types that are new in 64bits only OS are generated differently on 32bits
bindings. They mainly throw a PlatformNotSupportedException so it's
easier to diagnose (than a crash) what's happening at runtime.

This works well in all cases except one. When a new type, let's say
UIMenuElement is added and serves as a new base type for existing
types.

UIKeyCommand (iOS 7) -> UICommand (iOS 13)-> UIMenuElement (iOS 13)

This is correct as new base types can be added (in ObjC and C#).
However the generated code for the constructors of UICommand and
UIMenuElement would be throwing a PlatformNotSupportedException
which breaks the UIKeyCommand on 32 bits devices.

We fixed this in a few places by tweaking the availability attribute
but that requires spotting the new base type while doing bindings and
that is error prone [1][2].

This PR simply does let the protected constructor, using when chaining,
be generated normally. It's simpler and will cover all the cases (without
requiring hacks in the availability of those types)

[1] #7083
[2] #7084

… .ctor

Types that are new in 64bits only OS are generated differently on 32bits
bindings. They mainly throw a `PlatformNotSupportedException` so it's
easier to diagnose (than a crash) what's happening at runtime.

This works well in all cases except one. When a new type, let's say
`UIMenuElement` is added **and** serves as a new base type for existing
types.

`UIKeyCommand` (iOS 7) -> `UICommand` (iOS 13)-> `UIMenuElement` (iOS 13)

This is _correct_ as new base types can be added (in ObjC and C#).
However the generated code for the constructors of `UICommand` and
`UIMenuElement` would be throwing a `PlatformNotSupportedException`
which breaks the `UIKeyCommand` on 32 bits devices.

We fixed this in a few places by tweaking the availability attribute
but that requires spotting the new base type while doing bindings and
that is error prone [1][2].

This PR simply does let the `protected` constructor, using when chaining,
be generated normally. It's simpler and will cover all the cases (without
requiring hacks in the availability of those types)

[1] dotnet#7083
[2] dotnet#7084
@spouliot spouliot added this to the xcode11.1 milestone Sep 24, 2019
@spouliot spouliot added bug If an issue is a bug or a pull request a bug fix regression The issue or pull request is a regression labels Sep 24, 2019
Copy link
Contributor

@chamons chamons left a comment

Choose a reason for hiding this comment

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

❤️ test

@monojenkins
Copy link
Collaborator

Build failure
Build succeeded
API Diff (from stable)
API Diff (from PR only) (no change)
ℹ️ Generator Diff (please review changes)
🔥 Test run failed 🔥

Test results

7 tests failed, 81 tests passed.

Failed tests

  • xammac tests/Mac Modern/Debug: BuildFailure
  • xammac tests/Mac Modern/Release: BuildFailure
  • xammac tests/Mac Modern/Release: BuildFailure
  • monotouch-test/watchOS 32-bits - simulator/Debug: BuildFailure
  • monotouch-test/watchOS 32-bits - simulator/Debug (LinkSdk): BuildFailure
  • monotouch-test/watchOS 32-bits - simulator/Debug (static registrar): BuildFailure
  • monotouch-test/watchOS 32-bits - simulator/Release (all optimizations): BuildFailure

@monojenkins
Copy link
Collaborator

Build success
Build succeeded
API Diff (from stable)
API Diff (from PR only) (no change)
ℹ️ Generator Diff (please review changes)
Test run succeeded

@spouliot spouliot merged commit 6bdce2d into dotnet:xcode11.1 Sep 24, 2019
@spouliot spouliot deleted the xcode11.1-new-base-types branch September 24, 2019 20:24
@spouliot
Copy link
Contributor Author

@monojenkins backport to master

VincentDondain pushed a commit to VincentDondain/xamarin-macios that referenced this pull request Oct 16, 2019
… .ctor (dotnet#7085)


Types that are new in 64bits only OS are generated differently on 32bits
bindings. They mainly throw a `PlatformNotSupportedException` so it's
easier to diagnose (than a crash) what's happening at runtime.

This works well in all cases except one. When a new type, let's say
`UIMenuElement` is added **and** serves as a new base type for existing
types.

`UIKeyCommand` (iOS 7) -> `UICommand` (iOS 13)-> `UIMenuElement` (iOS 13)

This is _correct_ as new base types can be added (in ObjC and C#).
However the generated code for the constructors of `UICommand` and
`UIMenuElement` would be throwing a `PlatformNotSupportedException`
which breaks the `UIKeyCommand` on 32 bits devices.

We fixed this in a few places by tweaking the availability attribute
but that requires spotting the new base type while doing bindings and
that is error prone [1][2].

This PR simply does let the `protected` constructor, using when chaining,
be generated normally. It's simpler and will cover all the cases (without
requiring hacks in the availability of those types)

[1] dotnet#7083
[2] dotnet#7084
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug If an issue is a bug or a pull request a bug fix regression The issue or pull request is a regression
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants