Skip to content

make posix.Mode distinct #24964

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

Open
wants to merge 2 commits into
base: devel
Choose a base branch
from
Open

Conversation

Graveflo
Copy link
Contributor

After experiencing problems similar to #19374 this has been a common denominator. I don't know if this is the root of the problem but maybe it's better to use distinct when wrapping C code anyway.

@Araq
Copy link
Member

Araq commented May 21, 2025

That's a breaking change. Instead the generic instantiation cache should distinguish between Option[uint32] and Option[Mode].

@Graveflo
Copy link
Contributor Author

Graveflo commented May 21, 2025

That's a breaking change. Instead the generic instantiation cache should distinguish between Option[uint32] and Option[Mode].

I don't disagree but a couple of things:

  • I assume this is only desirable for aliases with pragmas, or some pragmas like importc ?
  • uint32 and mode_t do seem to be the same type in C on my machine and the cache usually doesn't miss, making this a non-issue most of the time. The problem is sometimes it actually does create two instantiations, but I haven't figured out how to trigger this reliably. If the opposite (never cache miss) were to be implemented and importc on an alias was to mean "this is the same type on the backend as it is in Nim" it should also work.
  • In my experience it's actually hard to tell the difference between built-in-style type nodes and aliases of them as they are almost identical. Maybe I'm missing something, but last time I looked that is was not fun.

From Nim's perspective unifying the instantiations of non-distinct aliases makes sense. If there is a bug in that it should probably be fixed; idk if it's localized to Mode and things like it. If the std is not going to be patched to use distinct where it probably should have in the past due to code breakages I have another suggestion that seems like the right answer, but I think it's been brought up before and I don't want to harp on it. I'm talking about "weak distinct". Meaning a type that is not an alias but a "subtype" of form to another. They are unique in instantiation but they match to their base type. This means that overloads and bindings for their base match the weak distinct, but bindings and overloads for the sub type do not match the base. If importc aliases were made implicitly weakly distinct it would solve all instances of this problem in the std, as well as add a useful tool to Nim.
edit: It very well may break stuff too 😆 I'm not exactly sure how it would play out. Currently the breakage that seems obvious are code that uses say seq[Mode] and seq[uint32] interchangeably, but this is also broken by the instantiation cache changes mentioned RE: "Instead the generic instantiation cache should distinguish between .."

@Araq
Copy link
Member

Araq commented May 21, 2025

If importc aliases were made implicitly weakly distinct it would solve all instances of this problem in the std, as well as add a useful tool to Nim.

Effectively I propose that an importc basic type is not an alias at all, it's a new type that happens to accept integer values.

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.

2 participants