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 skip for conversions in array indices in transf #24424

Merged
merged 2 commits into from
Nov 11, 2024

Conversation

metagn
Copy link
Collaborator

@metagn metagn commented Nov 9, 2024

fixes #17958

In transf, conversions in subscript expressions are skipped (with skipConv's rules). This is because array indexing can produce conversions to the range type that is the array's index type, which causes a RangeDefect rather than an IndexDefect (and also --rangeChecks and --indexChecks are both considered). However this causes problems when explicit conversions are used, between types of different bitsizes, because those also get skipped.

To fix this, we only skip the conversion if:

  • it's a hidden (implicit) conversion
  • it's a range check conversion (produces nkChckRange)
  • the subscript is on an array type and the result type of the conversion has the same bounds as the array index type

And skipConv rules also still apply (int/float classification).

Another idea would be to prevent the implicit conversion to the array index type from being generated. But there is no good way to do this: matching to the base type instead prevents types like uint32 from implicitly converting (i.e. it can convert to range[0..3] but not int), and analyzing whether this is an array bound check is easier in transf, since sigmatch just produces a type conversion.

The rules for skipping the conversion could also receive some other tweaks: We could add a rule that changing bitsizes also doesn't skip the conversion, but this breaks the uint32 case. We could simplify it to only removing implicit skips to specifically fix #17958, but this is wrong in general.

We could also add something like nkChckIndex that generates index errors instead but this is weird when it doesn't have access to the collection type and it might be overkill.

@metagn

This comment was marked as outdated.

@metagn metagn marked this pull request as draft November 9, 2024 16:56
@metagn metagn changed the title don't skip conversions in array indices in transf stricter skip for conversions in array indices in transf Nov 10, 2024
@metagn metagn marked this pull request as ready for review November 10, 2024 08:22
@Araq Araq merged commit 76c5f16 into nim-lang:devel Nov 11, 2024
18 checks passed
Copy link
Contributor

Thanks for your hard work on this PR!
The lines below are statistics of the Nim compiler built from 76c5f16

Hint: mm: orc; opt: speed; options: -d:release
177412 lines; 8.772s; 652.547MiB peakmem

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.

array[uint8, uint8] vs array[256, uint8]
2 participants