Skip to content

Conversation

@jjonescz
Copy link
Member

No description provided.

@jjonescz jjonescz requested review from 333fred and jcouv December 15, 2025 17:08
@jjonescz jjonescz requested a review from a team as a code owner December 15, 2025 17:08
Comment on lines 159 to 163
Only `extern` methods which are recognizable from metadata are handled, i.e., methods with the `pinvokeimpl` flag in IL.
Therefore, `extern` methods without `DllImport` attribute are not recognized as implicitly `unsafe` when used from a different assembly.
Such `extern` methods exposed as API should not be a common scenario anyway and there is already a warning for `extern` methods without any attribute
(albeit absence of that warning doesn't guarantee the method will be recognized as `unsafe` in metadata).
One can mark such methods as `unsafe` explicitly if needed.
Copy link
Member

Choose a reason for hiding this comment

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

Why would this be a caveat? The goal is that any extern method is unsafe, period.

Copy link
Member Author

Choose a reason for hiding this comment

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

extern without DllImport are not useful anyway I think and we would need to synthesize the RequiresUnsafe attribute for them. Which also means it would be different if such API was compiled by old or new compiler (whereas as currently proposed, even extern apis compiled by old compilers would be recognized in the same way).

Copy link
Member

Choose a reason for hiding this comment

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

Why would we need to synthesize anything for them? They're extern, they're just always unsafe.

Copy link
Member

Choose a reason for hiding this comment

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

Why would we need to synthesize anything for them? They're extern, they're just always unsafe.

Copy link
Member Author

@jjonescz jjonescz Dec 15, 2025

Choose a reason for hiding this comment

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

Because we can't tell whether they are extern from metadata, that's what I'm trying to say by this PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

Extern methods without any attribute look just like plain ordinary empty methods in IL.

Copy link
Member

@jkotas jkotas Dec 15, 2025

Choose a reason for hiding this comment

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

I think you should be able to tell by checking whether the method has IL implementation (method is implemented by IL).

If the method has no implementation in IL and it is not abstract (there may be a few more cases like that), it is extern -> unsafe.

Copy link
Member

Choose a reason for hiding this comment

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

extern without DllImport are not useful anyway

extern without DllImport are used by https://learn.microsoft.com/en-us/dotnet/api/system.runtime.compilerservices.unsafeaccessorattribute

Copy link
Member Author

Choose a reason for hiding this comment

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

You're right, it seems we can recognize extern methods by them having RVA 0, thanks.

Copy link
Member Author

@jjonescz jjonescz Dec 16, 2025

Choose a reason for hiding this comment

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

Not sure what to do with this: object.GetType (and probably other methods) on .net framework is marked as extern, so it would be an error to use it outside unsafe under the new rules.

https://github.com/microsoft/referencesource/blob/ec9fa9ae770d522a5b5f0607898044b7478574a3/mscorlib/system/object.cs#L105

@jjonescz jjonescz marked this pull request as draft December 16, 2025 10:11
@jjonescz jjonescz marked this pull request as ready for review December 16, 2025 15:08
@jjonescz jjonescz marked this pull request as draft December 17, 2025 15:14
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