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

Fix Bugzilla 24582 - Detect unsafe cast(bool[]) #16558

Merged
merged 1 commit into from
Jun 13, 2024
Merged

Conversation

ntrel
Copy link
Contributor

@ntrel ntrel commented Jun 3, 2024

Deprecate in safe code.

@dlang-bot
Copy link
Contributor

dlang-bot commented Jun 3, 2024

Thanks for your pull request and interest in making D better, @ntrel! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please verify that your PR follows this checklist:

  • My PR is fully covered with tests (you can see the coverage diff by visiting the details link of the codecov check)
  • My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
  • I have provided a detailed rationale explaining my changes
  • New or modified functions have Ddoc comments (with Params: and Returns:)

Please see CONTRIBUTING.md for more information.


If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment.

Bugzilla references

Auto-close Bugzilla Severity Description
24582 major Detect unsafe `cast(bool[])`

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub run digger -- build "master + dmd#16558"

ntrel added a commit to ntrel/phobos that referenced this pull request Jun 3, 2024
These are disallowed in `@safe` code with
dlang/dmd#16558.
dlang-bot pushed a commit to dlang/phobos that referenced this pull request Jun 3, 2024
These are disallowed in `@safe` code with
dlang/dmd#16558.
@thewilsonator
Copy link
Contributor

Dependant PR merged

@ntrel ntrel marked this pull request as draft June 3, 2024 16:34
@dkorpel
Copy link
Contributor

dkorpel commented Jun 3, 2024

Like I mentioned on bugzilla, I don't see how these casts are unsafe.

@ntrel ntrel changed the title Fix Bugzilla 24582 - Detect unsafe casting to bool Fix Bugzilla 24582 - Detect unsafe cast(bool[]) Jun 3, 2024
@ntrel
Copy link
Contributor Author

ntrel commented Jun 3, 2024

@dkorpel I added a comment there with an example:
https://issues.dlang.org/show_bug.cgi?id=24582#c3

Only the runtime array cast is unsafe, the other 2 are OK - thanks. I need to update this pull to allow the literal cast, as well as change the error to a deprecation.

@ntrel ntrel marked this pull request as ready for review June 5, 2024 10:40
@RazvanN7
Copy link
Contributor

RazvanN7 commented Jun 7, 2024

Why is the cast being deprecated? Wouldn't it make more sense to simply fix the literal cast to create an array of 0 and 1s? For the sake of consistency, casting a literal or a variable shouldn't yield different results.

@dkorpel
Copy link
Contributor

dkorpel commented Jun 10, 2024

Why is the cast being deprecated?

It's not, it's disallowed in @safe code.

Wouldn't it make more sense to simply fix the literal cast to create an array of 0 and 1s?

That's what happens.

For the sake of consistency, casting a literal or a variable shouldn't yield different results.

Currently it does, but changing that behavior is a breaking change and out of scope for this bug report.

@ntrel

This comment was marked as resolved.

@ntrel
Copy link
Contributor Author

ntrel commented Jun 10, 2024

Wouldn't it make more sense to simply fix the literal cast to create an array of 0 and 1s

That would go against the spec:

https://dlang.org/spec/expression.html#cast_array

The cast is done as a type paint

So the raw data is not modified.

@dkorpel
Copy link
Contributor

dkorpel commented Jun 10, 2024

No, as I posted on bugzilla, the byte contents is unchanged:

For the runtime cast yes, but the literal cast is different:

immutable c = cast(bool[]) [2, 4]; // literal cast applies to each element
pragma(msg, c); // [true, true]

@ntrel
Copy link
Contributor Author

ntrel commented Jun 10, 2024

@dkorpel OK, sorry - so we are in agreement!

REQUIRED_ARGS: -de
TEST_OUTPUT:
---
fail_compilation/bool_cast.d(12): Deprecation: cast from `ubyte[]` to `bool[]` not allowed in safe code
Copy link
Contributor

Choose a reason for hiding this comment

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

(Not that you need to do this right away, but) can we link to/refer users to the spec section/other official documentation, as to why this is an unsafe operation? It was certainly not immediately obvious to me as to why this is the case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've added a supplemental message for now.

Allow literal cast

Make deprecation

Add specific supplemental message
@ntrel
Copy link
Contributor Author

ntrel commented Jun 12, 2024

Tests now passing (other than 1 interop network request failure)

@dkorpel dkorpel merged commit 23e6712 into dlang:master Jun 13, 2024
41 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants