[opengl] Add nullability to (generated and manual) bindings #15174

tj-devel709 · 2022-06-01T17:24:52Z

This PR aims to bring nullability changes to OpenGL.
Following the steps here:

I am adding nullable enable to all manual files that are not "API_SOURCES" in src/frameworks.sources and making the required nullability changes
Changing all throw new ArgumentNullException ("object")); to ObjCRuntime.ThrowHelper.ThrowArgumentNullException (nameof (object)); for size saving optimization as well to mark that this framework contains nullability changes
Changing any == null or != null to is null and is not null

rolfbjarne

~~Looks like this is only whitespace changes, so not really needed?~~

~~https://github.com/xamarin/xamarin-macios/pull/15174/files/93c7310e51ddbeb45192d5d9702efc581bffc987..4ee26674163227a1e9794b4a15d15f6e310dcb14?w=1~~

NVM, just saw a portion of the PR

src/OpenGL/OpenTK/Math/Quaterniond.cs

src/OpenGL/OpenTK/Math/Matrix3d.cs

src/OpenGL/OpenTK/Math/Matrix3.cs

src/OpenGL/OpenTK/Math/Matrix2.cs

src/OpenGL/MonoMacGameView.cs

tj-devel709 · 2022-06-02T14:23:04Z

@rolfbjarne Changes addressed!

chamons

In general LGTM but I'm concerned that some of these changes are non-trivial, and we have literally no coverage/expected usage of some of this code.

Who is using MonoMacGameView.cs these days?

On one hand, that makes it lower risk (as few people will be impacted), but on the other hand we have likely zero sample coverage and it could stay broken awhile.

I think this is fine, but just something for us to consider.

tj-devel709 · 2022-06-06T15:17:17Z

Unrelated Test Failures: https://github.com/xamarin/maccore/issues/2558

mandel-macaque

@chamons I have the same though, the AssertContext was removed, which changes a behaviour in which we are throwing an exception, yet if you look closely what @tj-devel709 did was the following:

Remove the method.
Add the logic of the method to the property.

The change makes sense, since we are using now the property to access the OpenGL context, yet it has an undesired runtime effect. Our users used to be able to get a null variable out of the properties, now they are getting an exception. My fear here is that a user would be doing something like this:

if (view. OpenGLContext == null) {
    // call the init code
}

The above code, with the changes, will not work because it is throwing an exception. Now, issue here is that the Assert method is not very helpful with regards of nullability and hence the change from @tj-devel709 I propose something in between:

Revert the property change to not throw and return null
Add a private property that does the new logic, and call it _OpenGLContext

With the above, you have what @tj-devel709 was looking for and what @chamons wants (no runtime changes).

chamons · 2022-06-06T15:37:36Z

Or you could could it GetOpenGLContext() or ActualOpenGLContext. I'm not a huge fan of underscores having different behavior.

mandel-macaque · 2022-06-15T15:29:35Z

src/OpenGL/MonoMacGameView.cs

+			get {
+				if (pixelFormat is null)
+					throw new InvalidOperationException ("Operation requires a PixelFormat that cannot be null.");
+				return pixelFormat;


This property has AFAIK the same issue as the OpenGL one we talked about. You are throwing an exception on a property that did not used to and returned a null variable correctly. We are changing the behaviour and we should not.

mandel-macaque · 2022-06-15T15:30:57Z

src/OpenGL/MonoMacGameView.cs

+			get => openGLContext;
+		}
+
+		public NSOpenGLContext ActualOpenGLContext {


This should not be public, you are adding a public property that is not needed since we have the OpenGLContext. This is an implementation detail that we are leaking to the users.

Suggested change

public NSOpenGLContext ActualOpenGLContext {

private NSOpenGLContext ActualOpenGLContext {

vs-mobiletools-engineering-service2 · 2022-06-15T17:47:04Z

💻 [PR Build] Tests on macOS Mac Catalina (10.15) passed 💻

✅ All tests on macOS Mac Catalina (10.15) passed.

Pipeline on Agent
Hash: 45f301ad0e78d68a592a731002333acc2b628a25 [PR build]

vs-mobiletools-engineering-service2 · 2022-06-15T17:50:50Z

📚 [PR Build] Artifacts 📚

Packages generated

View packages

Pipeline on Agent XAMBOT-1030.Monterey'
Hash: 45f301ad0e78d68a592a731002333acc2b628a25 [PR build]

vs-mobiletools-engineering-service2 · 2022-06-15T17:52:23Z

✅ API diff for current PR / commit

Legacy Xamarin (No breaking changes)

iOS: vsdrops gist (No breaking changes)
tvOS: vsdrops gist (No breaking changes)
watchOS: vsdrops gist (No breaking changes)
macOS: vsdrops gist (No breaking changes)

.NET (No breaking changes)

iOS: vsdrops gist (No breaking changes)
tvOS: vsdrops gist (No breaking changes)
MacCatalyst: vsdrops gist (No breaking changes)
macOS: vsdrops gist (No breaking changes)

✅ API diff vs stable

Legacy Xamarin (No breaking changes)

iOS: vsdrops gist (No breaking changes)
tvOS: vsdrops gist (No breaking changes)
watchOS: vsdrops gist (No breaking changes)
macOS: vsdrops gist (No breaking changes)

.NET (No breaking changes)

iOS: vsdrops gist (No breaking changes)
tvOS: vsdrops gist (No breaking changes)
MacCatalyst: vsdrops gist (No breaking changes)
macOS: vsdrops gist (No breaking changes)
Microsoft.iOS vs Microsoft.MacCatalyst: vsdrops gist

Legacy Xamarin (stable) vs .NET

iOS: vsdrops gist
tvOS: vsdrops gist
macOS: vsdrops gist

ℹ️ Generator diff

Generator Diff: vsdrops (html) vsdrops (raw diff) gist (raw diff) - Please review changes)

Pipeline on Agent
Hash: 45f301ad0e78d68a592a731002333acc2b628a25 [PR build]

vs-mobiletools-engineering-service2 · 2022-06-15T18:10:56Z

❌ [PR Build] Tests on macOS M1 - Mac Big Sur (11.5) failed ❌

Failed tests are:

xammac_tests
monotouch-test

Pipeline on Agent
Hash: 45f301ad0e78d68a592a731002333acc2b628a25 [PR build]

vs-mobiletools-engineering-service2 · 2022-06-16T00:46:27Z

❌ [CI Build] Tests failed on VSTS: simulator tests iOS ❌

Tests failed on VSTS: simulator tests iOS.

Test results

1 tests failed, 147 tests passed.

Failed tests

introspection/watchOS 32-bits - simulator/Debug: Crashed

Pipeline on Agent XAMBOT-1030.Monterey'
Merge 45f301a into 011f977

tj-devel709 · 2022-06-16T22:20:15Z

Unrelated Test Failure: https://github.com/xamarin/maccore/issues/2558

tj_devel709 added 3 commits May 31, 2022 21:03

Enable Nullability

a4e47aa

throw better exceptions

4b2a47f

use is null

93c7310

tj-devel709 added the not-notes-worthy Ignore for release notes label Jun 1, 2022

tj-devel709 added this to the Future milestone Jun 1, 2022

tj_devel709 and others added 2 commits June 1, 2022 12:25

Style fixes

4ee2667

Merge branch 'main' into Nullable-OpenGL

e3933c5

rolfbjarne reviewed Jun 1, 2022

View reviewed changes

rolfbjarne approved these changes Jun 1, 2022

View reviewed changes

This comment has been minimized.

Sign in to view

tj_devel709 added 2 commits June 2, 2022 09:22

address Rolf suggestions

5943907

Merge remote-tracking branch 'TJ/Nullable-OpenGL' into Nullable-OpenGL

12afd33

This comment has been minimized.

Sign in to view

chamons approved these changes Jun 6, 2022

View reviewed changes

mandel-macaque requested changes Jun 6, 2022

View reviewed changes

use better syntax

f221dd7

This comment has been minimized.

Sign in to view

mandel-macaque requested changes Jun 15, 2022

View reviewed changes

Merge branch 'main' into Nullable-OpenGL

9788e93

tj_devel709 added 2 commits June 15, 2022 11:04

make properties private and use ActualOpenGLContext

345a69c

Merge remote-tracking branch 'TJ/Nullable-OpenGL' into Nullable-OpenGL

45f301a

mandel-macaque approved these changes Jun 15, 2022

View reviewed changes

tj-devel709 merged commit a1e53d6 into dotnet:main Jun 17, 2022

	public NSOpenGLContext ActualOpenGLContext {
	private NSOpenGLContext ActualOpenGLContext {

[opengl] Add nullability to (generated and manual) bindings #15174

[opengl] Add nullability to (generated and manual) bindings #15174

Uh oh!

Conversation

tj-devel709 commented Jun 1, 2022

Uh oh!

rolfbjarne left a comment • 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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

tj-devel709 commented Jun 2, 2022

Uh oh!

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

chamons left a comment

Choose a reason for hiding this comment

Uh oh!

tj-devel709 commented Jun 6, 2022

Uh oh!

mandel-macaque left a comment

Choose a reason for hiding this comment

Uh oh!

chamons commented Jun 6, 2022

Uh oh!

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

mandel-macaque Jun 15, 2022

Choose a reason for hiding this comment

Uh oh!

mandel-macaque Jun 15, 2022

Choose a reason for hiding this comment

Uh oh!

tj-devel709 Jun 15, 2022

Choose a reason for hiding this comment

Uh oh!

vs-mobiletools-engineering-service2 commented Jun 15, 2022

💻 [PR Build] Tests on macOS Mac Catalina (10.15) passed 💻

Uh oh!

vs-mobiletools-engineering-service2 commented Jun 15, 2022

📚 [PR Build] Artifacts 📚

Packages generated

Uh oh!

vs-mobiletools-engineering-service2 commented Jun 15, 2022

✅ API diff for current PR / commit

✅ API diff vs stable

ℹ️ Generator diff

Uh oh!

vs-mobiletools-engineering-service2 commented Jun 15, 2022

❌ [PR Build] Tests on macOS M1 - Mac Big Sur (11.5) failed ❌

Uh oh!

vs-mobiletools-engineering-service2 commented Jun 16, 2022

❌ [CI Build] Tests failed on VSTS: simulator tests iOS ❌

Test results

Failed tests

Uh oh!

tj-devel709 commented Jun 16, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

rolfbjarne left a comment •

edited

Loading