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

stricter var/pointer type conversions, subranges under preview define #24038

Open
wants to merge 1 commit into
base: devel
Choose a base branch
from

Conversation

metagn
Copy link
Collaborator

@metagn metagn commented Aug 31, 2024

closes #24032, split from #24036

var/pointer types are no longer implicitly convertible to each other if their element types either:

  • require an int conversion or another conversion operation as long as it's not to openarray,
  • are subtypes with pointer indirection,
  • are a base type/subrange pair if -d:nimPreviewStrictVarRange is not defined.

Previously any conversion below a subrange match would match if the element type wasn't a pointer type.

The range mismatch is opt-in for now because:

  1. it breaks a lot of code, things like +=/-=/atomicInc/fromJson etc aren't implemented for range types,
  2. there aren't any mechanisms for addressably converting ranges to their base types, allow conversions between var types of range types and base types #24037 adds one, there also isn't a rangeBase typetrait or a typeclass for range types with a specific base type

So more work is needed before this is usable.

Only normalize needs to be patched for the non-opt-in changes: nitely/nim-normalize#9

@beef331
Copy link
Collaborator

beef331 commented Aug 31, 2024

Any chance this could emit a warning without the define, that way it does not surprise anyone when it eventually becomes the default?

@metagn
Copy link
Collaborator Author

metagn commented Aug 31, 2024

Didn't think of that, will look into it. My intention was to include in the mismatch error message "incompatible range type, convert to base type doing int(foo)" or whatever but there's not even an alternative you can suggest yet. I only made this PR so it could be standalone from the draft, we could technically merge this since it's opt-in but realistically it won't be usable until like 3 more PRs. And because it breaks so much code it won't be default for a while and will most likely be a legacy switch even when it is default.

So there shouldn't be too much cause for alarm, I tried to word the changelog so it wouldn't seem urgent but I don't know. It would be a lot easier to explain it if we had concrete alternatives.

metagn added a commit to metagn/Nim that referenced this pull request Sep 3, 2024
Araq pushed a commit that referenced this pull request Sep 6, 2024
refs #21682, refs #24038

The `rangeBase` typetrait added in #21682 which gives the base type of a
range type is now added publicly to `typetraits`. Previously it was only
privately used in `repr_v2` and in `enumutils` since #24052
(coincidentally I didn't see this until now). This is part of an effort
to make range types easier to work with in generics, as mentioned in
#24038. Its use combined with #24037 is also tested.

The condition for the "enum to enum conversion" warning is now also
restricted to conversions between different enum base types, i.e.
conversion between an enum type and a range type of itself doesn't give
a warning. I put this in this PR since the test gave the warning and so
works as a regression test.
Araq pushed a commit that referenced this pull request Sep 18, 2024
…24130)

split again from #24038, fixes
status-im/nimbus-eth2#6554 (comment)

`var`/pointer types are no longer implicitly convertible to each other
if their element types either:

* require an int conversion or another conversion operation as long as
it's not to `openarray`,
* are subtypes with pointer indirection,

Previously any conversion below a subrange match would match if the
element type wasn't a pointer type, then it would error later in
`analyseIfAddressTaken`.

Different from #24038 in that the preview define that made subrange
matches also fail to match is removed for a simpler diff so that it can
be backported.
narimiran pushed a commit that referenced this pull request Sep 18, 2024
…24130)

split again from #24038, fixes
status-im/nimbus-eth2#6554 (comment)

`var`/pointer types are no longer implicitly convertible to each other
if their element types either:

* require an int conversion or another conversion operation as long as
it's not to `openarray`,
* are subtypes with pointer indirection,

Previously any conversion below a subrange match would match if the
element type wasn't a pointer type, then it would error later in
`analyseIfAddressTaken`.

Different from #24038 in that the preview define that made subrange
matches also fail to match is removed for a simpler diff so that it can
be backported.

(cherry picked from commit 1660ddf)
metagn added a commit to metagn/Nim that referenced this pull request Sep 21, 2024
…im-lang#24130)

split again from nim-lang#24038, fixes
status-im/nimbus-eth2#6554 (comment)

`var`/pointer types are no longer implicitly convertible to each other
if their element types either:

* require an int conversion or another conversion operation as long as
it's not to `openarray`,
* are subtypes with pointer indirection,

Previously any conversion below a subrange match would match if the
element type wasn't a pointer type, then it would error later in
`analyseIfAddressTaken`.

Different from nim-lang#24038 in that the preview define that made subrange
matches also fail to match is removed for a simpler diff so that it can
be backported.
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.

Unexpected behavior with range types and custom operators
2 participants