Skip to content

Conversation

@rolfbjarne
Copy link
Member

@rolfbjarne rolfbjarne commented Jan 19, 2023

This PR handles two scenarios (fixed in separate commits):

Scenario 1:

  • The property has different availability attributes than the containing type.
  • The property's accessor(s) do not have availability attributes.

We'd generate the wrong availability attributes for the property accessors,
because we'd take the type's availability attributes and add them to the
accessors.

As for the fix: I can't really explain it. This code is rather impenetrable,
and the parameter names don't make much sense, but whatever I did seems to
work?

And it turns out this fix shows up in an existing test as well (the
generator's Bug35176 test), which I had to modify to remove the expectation of
(now redundant) availability attributes that we no longer produce.

Scenario 2:

  • Type is available on iOS, tvOS.
  • Property in the type is available on iOS (and not tvOS).
  • Property accessor has explicit availability attributes for iOS.

Then the property accessor would get the availability attribute for tvOS from the
type, and not the (un)availability attribute from the property.

The fix is to make sure the parent context is the property (and not the type) when
processing availability attributes for the accessor.

@vs-mobiletools-engineering-service2

This comment has been minimized.

@vs-mobiletools-engineering-service2

This comment has been minimized.

@vs-mobiletools-engineering-service2

This comment has been minimized.

@vs-mobiletools-engineering-service2

This comment has been minimized.

@rolfbjarne rolfbjarne force-pushed the generator-accessor-fix branch from 873eff7 to c9ace05 Compare January 20, 2023 07:54
@vs-mobiletools-engineering-service2

This comment has been minimized.

@vs-mobiletools-engineering-service2

This comment has been minimized.

@vs-mobiletools-engineering-service2

This comment has been minimized.

@vs-mobiletools-engineering-service2

This comment has been minimized.

@vs-mobiletools-engineering-service2

This comment has been minimized.

@vs-mobiletools-engineering-service2

This comment has been minimized.

@vs-mobiletools-engineering-service2

This comment has been minimized.

@vs-mobiletools-engineering-service2

This comment has been minimized.

@vs-mobiletools-engineering-service2

This comment has been minimized.

@vs-mobiletools-engineering-service2

This comment has been minimized.

@vs-mobiletools-engineering-service2

This comment has been minimized.

@vs-mobiletools-engineering-service2

This comment has been minimized.

@vs-mobiletools-engineering-service2

This comment has been minimized.

@vs-mobiletools-engineering-service2

This comment has been minimized.

@vs-mobiletools-engineering-service2

This comment has been minimized.

@vs-mobiletools-engineering-service2

This comment has been minimized.

@rolfbjarne rolfbjarne marked this pull request as ready for review January 24, 2023 06:54
@rolfbjarne rolfbjarne requested a review from dalexsoto as a code owner January 24, 2023 06:54
…availability is different than the property itself.

In the following scenario:

* A property is available on iOS (and implicitly Mac Catalyst).
* One of the property accessors is not available on iOS, but still available on Mac Catalyst (explicitly).

We'd generate the wrong availability attributes for ...

As for the fix: I can't really explain it. This code is rather impenetrable,
and the parameter names don't make much sense, but whatever I did seems to
work?

And it turns out this fix shows up in an existing test as well (the
generator's Bug35176 test), which I had to modify to remove the expectation of
(redundant) availability attributes that we no longer produce.
… the property itself.

Scenario:

* Type is available on iOS, tvOS.
* Property in the type is available on iOS (and not tvOS).
* Property accessor has explicit availability attributes for iOS.

Then the property accessor would get the availability attribute for tvOS from the
type, and not the (un)availability attribute from the property.

The fix is to make sure the parent context is the property (and not the type) when
processing availability attributes for the accessor.
@rolfbjarne rolfbjarne force-pushed the generator-accessor-fix branch from 967737f to be7e8f5 Compare January 24, 2023 13:33
@vs-mobiletools-engineering-service2
Copy link
Collaborator

✅ API diff for current PR / commit

Legacy Xamarin (No breaking changes)
  • iOS (no change detected)
  • tvOS (no change detected)
  • watchOS (no change detected)
  • macOS (no change detected)
NET (empty diffs)
  • iOS: (empty diff detected)
  • tvOS: (empty diff detected)
  • MacCatalyst: (empty diff detected)
  • macOS: (empty diff detected)

✅ API diff vs stable

Legacy Xamarin (No breaking changes)
.NET (No breaking changes)
Legacy Xamarin (stable) vs .NET

ℹ️ Generator diff

Generator Diff: vsdrops (html) vsdrops (raw diff) Unable to create gist: Response status code does not indicate success: 502 (Bad Gateway). (raw diff) - Please review changes)

Pipeline on Agent
Hash: be7e8f57c74a6ea631147f59a9e4c31c6f2350b8 [PR build]

@vs-mobiletools-engineering-service2
Copy link
Collaborator

💻 [PR Build] Tests on macOS M1 - Mac Big Sur (11.5) passed 💻

All tests on macOS M1 - Mac Big Sur (11.5) passed.

Pipeline on Agent
Hash: be7e8f57c74a6ea631147f59a9e4c31c6f2350b8 [PR build]

@vs-mobiletools-engineering-service2
Copy link
Collaborator

📚 [PR Build] Artifacts 📚

Packages generated

View packages

Pipeline on Agent XAMBOT-1107.Monterey'
Hash: be7e8f57c74a6ea631147f59a9e4c31c6f2350b8 [PR build]

@vs-mobiletools-engineering-service2
Copy link
Collaborator

🚀 [CI Build] Test results 🚀

Test results

✅ All tests passed on VSTS: simulator tests.

🎉 All 225 tests passed 🎉

Tests counts

✅ bcl: All 69 tests passed. Html Report (VSDrops) Download
✅ cecil: All 1 tests passed. Html Report (VSDrops) Download
✅ dotnettests: All 1 tests passed. Html Report (VSDrops) Download
✅ fsharp: All 7 tests passed. Html Report (VSDrops) Download
✅ framework: All 8 tests passed. Html Report (VSDrops) Download
✅ generator: All 2 tests passed. Html Report (VSDrops) Download
✅ interdependent_binding_projects: All 7 tests passed. Html Report (VSDrops) Download
✅ install_source: All 1 tests passed. Html Report (VSDrops) Download
✅ introspection: All 8 tests passed. Html Report (VSDrops) Download
✅ linker: All 65 tests passed. Html Report (VSDrops) Download
✅ mac_binding_project: All 1 tests passed. Html Report (VSDrops) Download
✅ mmp: All 2 tests passed. Html Report (VSDrops) Download
✅ mononative: All 12 tests passed. Html Report (VSDrops) Download
✅ monotouch: All 25 tests passed. Html Report (VSDrops) Download
✅ msbuild: All 2 tests passed. Html Report (VSDrops) Download
✅ mtouch: All 1 tests passed. Html Report (VSDrops) Download
✅ xammac: All 3 tests passed. Html Report (VSDrops) Download
✅ xcframework: All 8 tests passed. Html Report (VSDrops) Download
✅ xtro: All 2 tests passed. Html Report (VSDrops) Download

Pipeline on Agent
Hash: be7e8f57c74a6ea631147f59a9e4c31c6f2350b8 [PR build]

@rolfbjarne rolfbjarne removed request for emaf and mauroa January 25, 2023 07:53
Copy link
Member

@dalexsoto dalexsoto left a comment

Choose a reason for hiding this comment

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

Tests give more confidence about the change 👍

@rolfbjarne rolfbjarne merged commit a57695b into dotnet:main Jan 25, 2023
@rolfbjarne rolfbjarne deleted the generator-accessor-fix branch January 25, 2023 08:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants