Add NonMaxU32 as integer variant for PropertyKey
#3321
Conversation
raskad
added
bug
Test262 conformance changes
Fixed tests (2): |
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #3321 +/- ##
==========================================
- Coverage 49.72% 49.34% -0.39%
==========================================
Files 443 445 +2
Lines 43378 43892 +514
==========================================
+ Hits 21570 21657 +87
- Misses 21808 22235 +427
☔ View full report in Codecov by Sentry. |
raskad
force-pushed
the
max-int-index
branch
from
e6854b1
to
d775d37
Compare
boa_engine/src/property/nonmaxu32.rs
Outdated
| pub const fn new_unchecked(inner: u32) -> Self { | ||
| debug_assert!(inner != u32::MAX); | ||
|
|
||
| Self { inner } | ||
| } |
There was a problem hiding this comment.
I think this should be an unsafe function. My reasoning is that, in the future, we could implement an optimization that assumes that the invariant holds. However, if we leave this function as safe, we'd have to make a breaking change to add unsafe at that moment. So, in preparation to that I'd mark it as unsafe.
There was a problem hiding this comment.
I would argue that if we need to change this to be unsafe later, we just do the breaking change. In my opinion it is more important to have a clean API that does what is expected, than to avoid breaking changes.
There was a problem hiding this comment.
It's not only a matter of doing the breaking change though. If, for example, someone tried to do an unsafe optimization assuming the invariant holds, and the invariant didn't hold, it would mean that a safe function would be the cause of UB.
There was a problem hiding this comment.
E.g. String::from_utf8_unchecked and its safety comment.
There was a problem hiding this comment.
Well but currently we do not have any optimizations based on the invariant, right? If we add some, we would have to mark the function as unsafe in that same change. In contrast I think there are unsafe optimizations in String that depend on the bytes being UTF-8.
There was a problem hiding this comment.
Well but currently we do not have any optimizations based on the invariant, right?
Note that the safety comment doesn't specify that there are uses of that method that unsafely assume the string is utf8:
If this constraint is violated, it may cause memory unsafety issues with future users [sic] of the String, ...
Emphasis on "... with future uses of the String"
If we add some, we would have to mark the function as unsafe in that same change.
I don't think it's ideal to rely on the fact that all of us have to remember to change the function to unsafe in the same PR.
There was a problem hiding this comment.
I think the String documentation is not talking about "future" in the sense of functionality that may or may be not added in the future but rather the future of the created String itself. I think the next sentence makes it a little more clear: [...] as the rest of the standard library assumes that Strings are valid UTF-8.
I don't think it's ideal to rely on the fact that all of us have to remember to change the function to unsafe in the same PR.
I very much disagree with this. When we add unsafe optimizations, we have to check that there is no way the constraints that the optimizations depend on can be violated outside of unsafe functions themselves. In this case, we would have to make sure that there is no way a NonMaxU32 can be constructed without a u32::MAX value, expect from an unsafe function that documents this constraint.
Nevermind all of that, let's not get too hung up on this. If you think the function should be unsafe, I will add it back and remove the debug_assert.
This Pull Request supersedes #2131
It changes the following:
NonMaxU32that is used as the integer variant forPropertyKey.