fix: Conversion operations having poison results #131

doug-q · 2024-10-20T10:44:08Z

Closes #103

doug-q · 2024-10-21T08:04:27Z

Failing beta check is fixed in #134.

doug-q · 2024-10-21T08:05:18Z

src/extension/conversions.rs

+    #[case(u64::MAX - (1 << 8))]
+    #[case(u64::MAX - (1 << 9))]
+    #[case(u64::MAX - (1 << 10))]
+    #[case(u64::MAX - (1 << 11))]


I'm not happy with these cases and those for the signed variant below. Any ideas how to improve this?

codecov-commenter · 2024-10-21T09:22:23Z

Codecov Report

Attention: Patch coverage is 90.69767% with 4 lines in your changes missing coverage. Please review.

Project coverage is 85.18%. Comparing base (ce14684) to head (3c20cd3).

Files with missing lines	Patch %	Lines
src/extension/conversions.rs	90.69%	2 Missing and 2 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #131      +/-   ##
==========================================
+ Coverage   85.13%   85.18%   +0.05%     
==========================================
  Files          31       31              
  Lines        4238     4247       +9     
  Branches     4238     4247       +9     
==========================================
+ Hits         3608     3618      +10     
  Misses        305      305              
+ Partials      325      324       -1

Flag	Coverage Δ
rust	`85.18% <90.69%> (+0.05%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

cqc-alec · 2024-10-21T09:52:42Z

src/extension/conversions.rs

+            i64::MIN >> (64 - width),
+            i64::MAX >> (64 - width),


These lines scare me a bit (I think in C shift operations on signed integers are "implementation dependent"?)

According to https://doc.rust-lang.org/reference/expressions/operator-expr.html#arithmetic-and-logical-binary-operators these are arithmetic right shift, but which way does rounding go? I couldn't find chapter and verse on this.

It's probably fine.

I don't think rounding is not the right way to think about it. Shifting is a bit operation, which happens to mean "divide by 2 rounding down, repeat". Instead, the bit pattern of i64::MIN is 1000.... Arithmetic right shifting(i.e. sign extending so that new most-significant-bits are 1) the 1 to the new most-significant-bit is the correct thing.
Similarly the bit pattern of i64::MAX is 0111111. Arithmetic right shifting (i.e. new most-significant-bits are 0) the 0 to the new most-significant-bit is the correct thing.

the bit pattern of i64::MIN is 1000...

Is this guaranteed?

Yes, I believe so. https://doc.rust-lang.org/book/ch03-02-data-types.html#integer-types says "Signed numbers are stored using two’s complement representation."

Thanks! All good then.

## 🤖 New release * `hugr-llvm`: 0.5.1 -> 0.6.0 (⚠️ API breaking changes) ### ⚠️ `hugr-llvm` breaking changes ``` --- failure auto_trait_impl_removed: auto trait no longer implemented --- Description: A public type has stopped implementing one or more auto traits. This can break downstream code that depends on the traits being implemented. ref: https://doc.rust-lang.org/reference/special-types-and-traits.html#auto-traits impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.36.0/src/lints/auto_trait_impl_removed.ron Failed in: type TypeConverter is no longer UnwindSafe, in /tmp/.tmpGjujM0/hugr-llvm/src/types.rs:59 type TypeConverter is no longer RefUnwindSafe, in /tmp/.tmpGjujM0/hugr-llvm/src/types.rs:59 --- failure derive_trait_impl_removed: built-in derived trait no longer implemented --- Description: A public type has stopped deriving one or more traits. This can break downstream code that depends on those types implementing those traits. ref: https://doc.rust-lang.org/reference/attributes/derive.html#derive impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.36.0/src/lints/derive_trait_impl_removed.ron Failed in: type TypeConverter no longer derives Copy, in /tmp/.tmpGjujM0/hugr-llvm/src/types.rs:59 type TypeConverter no longer derives Clone, in /tmp/.tmpGjujM0/hugr-llvm/src/types.rs:59 type TypeConverter no longer derives Debug, in /tmp/.tmpGjujM0/hugr-llvm/src/types.rs:59 --- failure function_missing: pub fn removed or renamed --- Description: A publicly-visible function cannot be imported by its prior path. A `pub use` may have been removed, or the function itself may have been renamed or removed entirely. ref: https://doc.rust-lang.org/cargo/reference/semver.html#item-remove impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.36.0/src/lints/function_missing.ron Failed in: function hugr_llvm::custom::prelude::array::emit_array_op, previously in file /tmp/.tmpu0X9jI/hugr-llvm/src/custom/prelude/array.rs:51 function hugr_llvm::custom::int::add_int_extensions, previously in file /tmp/.tmpu0X9jI/hugr-llvm/src/custom/int.rs:226 function hugr_llvm::custom::rotation::add_rotation_extensions, previously in file /tmp/.tmpu0X9jI/hugr-llvm/src/custom/rotation.rs:196 function hugr_llvm::custom::prelude::add_prelude_extensions, previously in file /tmp/.tmpu0X9jI/hugr-llvm/src/custom/prelude.rs:295 function hugr_llvm::custom::float::add_float_extensions, previously in file /tmp/.tmpu0X9jI/hugr-llvm/src/custom/float.rs:186 function hugr_llvm::custom::conversions::add_conversions_extension, previously in file /tmp/.tmpu0X9jI/hugr-llvm/src/custom/conversions.rs:270 function hugr_llvm::custom::logic::add_logic_extensions, previously in file /tmp/.tmpu0X9jI/hugr-llvm/src/custom/logic.rs:106 function hugr_llvm::custom::prelude::add_default_prelude_extensions, previously in file /tmp/.tmpu0X9jI/hugr-llvm/src/custom/prelude.rs:304 --- failure inherent_method_missing: pub method removed or renamed --- Description: A publicly-visible method or associated fn is no longer available under its prior name. It may have been renamed or removed entirely. ref: https://doc.rust-lang.org/cargo/reference/semver.html#item-remove impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.36.0/src/lints/inherent_method_missing.ron Failed in: TypeConverter::new, previously in file /tmp/.tmpu0X9jI/hugr-llvm/src/types.rs:107 TypeConverter::iw_context, previously in file /tmp/.tmpu0X9jI/hugr-llvm/src/types.rs:112 CodegenExtsMap::add_float_extensions, previously in file /tmp/.tmpu0X9jI/hugr-llvm/src/custom/float.rs:192 CodegenExtsMap::add_int_extensions, previously in file /tmp/.tmpu0X9jI/hugr-llvm/src/custom/int.rs:234 CodegenExtsMap::add_logic_extensions, previously in file /tmp/.tmpu0X9jI/hugr-llvm/src/custom/logic.rs:113 CodegenExtsMap::add_default_prelude_extensions, previously in file /tmp/.tmpu0X9jI/hugr-llvm/src/custom/prelude.rs:311 CodegenExtsMap::add_prelude_extensions, previously in file /tmp/.tmpu0X9jI/hugr-llvm/src/custom/prelude.rs:317 CodegenExtsMap::add_rotation_extensions, previously in file /tmp/.tmpu0X9jI/hugr-llvm/src/custom/rotation.rs:201 CodegenExtsMap::new, previously in file /tmp/.tmpu0X9jI/hugr-llvm/src/custom.rs:88 CodegenExtsMap::add_cge, previously in file /tmp/.tmpu0X9jI/hugr-llvm/src/custom.rs:97 CodegenExtsMap::get, previously in file /tmp/.tmpu0X9jI/hugr-llvm/src/custom.rs:110 CodegenExtsMap::llvm_type, previously in file /tmp/.tmpu0X9jI/hugr-llvm/src/custom.rs:120 CodegenExtsMap::emit, previously in file /tmp/.tmpu0X9jI/hugr-llvm/src/custom.rs:130 CodegenExtsMap::load_constant, previously in file /tmp/.tmpu0X9jI/hugr-llvm/src/custom.rs:145 --- failure module_missing: pub module removed or renamed --- Description: A publicly-visible module cannot be imported by its prior path. A `pub use` may have been removed, or the module may have been renamed, removed, or made non-public. ref: https://doc.rust-lang.org/cargo/reference/semver.html#item-remove impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.36.0/src/lints/module_missing.ron Failed in: mod hugr_llvm::custom::conversions, previously in file /tmp/.tmpu0X9jI/hugr-llvm/src/custom/conversions.rs:1 mod hugr_llvm::custom::logic, previously in file /tmp/.tmpu0X9jI/hugr-llvm/src/custom/logic.rs:1 mod hugr_llvm::custom::int, previously in file /tmp/.tmpu0X9jI/hugr-llvm/src/custom/int.rs:1 mod hugr_llvm::custom::prelude, previously in file /tmp/.tmpu0X9jI/hugr-llvm/src/custom/prelude.rs:1 mod hugr_llvm::fat, previously in file /tmp/.tmpu0X9jI/hugr-llvm/src/fat.rs:1 mod hugr_llvm::custom::float, previously in file /tmp/.tmpu0X9jI/hugr-llvm/src/custom/float.rs:1 mod hugr_llvm::custom::rotation, previously in file /tmp/.tmpu0X9jI/hugr-llvm/src/custom/rotation.rs:1 mod hugr_llvm::custom::prelude::array, previously in file /tmp/.tmpu0X9jI/hugr-llvm/src/custom/prelude/array.rs:1 --- failure struct_missing: pub struct removed or renamed --- Description: A publicly-visible struct cannot be imported by its prior path. A `pub use` may have been removed, or the struct itself may have been renamed or removed entirely. ref: https://doc.rust-lang.org/cargo/reference/semver.html#item-remove impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.36.0/src/lints/struct_missing.ron Failed in: struct hugr_llvm::custom::logic::LogicCodegenExtension, previously in file /tmp/.tmpu0X9jI/hugr-llvm/src/custom/logic.rs:78 struct hugr_llvm::fat::FatNode, previously in file /tmp/.tmpu0X9jI/hugr-llvm/src/fat.rs:28 struct hugr_llvm::custom::int::IntOpsCodegenExtension, previously in file /tmp/.tmpu0X9jI/hugr-llvm/src/custom/int.rs:120 struct hugr_llvm::custom::prelude::PreludeCodegenExtension, previously in file /tmp/.tmpu0X9jI/hugr-llvm/src/custom/prelude.rs:174 struct hugr_llvm::custom::float::FloatTypesCodegenExtension, previously in file /tmp/.tmpu0X9jI/hugr-llvm/src/custom/float.rs:27 struct hugr_llvm::emit::NullEmitLlvm, previously in file /tmp/.tmpu0X9jI/hugr-llvm/src/emit.rs:52 struct hugr_llvm::custom::conversions::ConversionsCodegenExtension, previously in file /tmp/.tmpu0X9jI/hugr-llvm/src/custom/conversions.rs:244 struct hugr_llvm::custom::prelude::DefaultPreludeCodegen, previously in file /tmp/.tmpu0X9jI/hugr-llvm/src/custom/prelude.rs:169 struct hugr_llvm::custom::int::IntTypesCodegenExtension, previously in file /tmp/.tmpu0X9jI/hugr-llvm/src/custom/int.rs:150 struct hugr_llvm::custom::rotation::RotationCodegenExtension, previously in file /tmp/.tmpu0X9jI/hugr-llvm/src/custom/rotation.rs:30 --- failure trait_method_added: pub trait method added --- Description: A non-sealed public trait added a new method without a default implementation, which breaks downstream implementations of the trait ref: https://doc.rust-lang.org/cargo/reference/semver.html#trait-new-item-no-default impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.36.0/src/lints/trait_method_added.ron Failed in: trait method hugr_llvm::custom::CodegenExtension::add_extension in file /tmp/.tmpGjujM0/hugr-llvm/src/custom.rs:41 --- failure trait_method_missing: pub trait method removed or renamed --- Description: A trait method is no longer callable, and may have been renamed or removed entirely. ref: https://doc.rust-lang.org/cargo/reference/semver.html#major-any-change-to-trait-item-signatures impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.36.0/src/lints/trait_method_missing.ron Failed in: method extension of trait CodegenExtension, previously in file /tmp/.tmpu0X9jI/hugr-llvm/src/custom.rs:38 method supported_consts of trait CodegenExtension, previously in file /tmp/.tmpu0X9jI/hugr-llvm/src/custom.rs:44 method llvm_type of trait CodegenExtension, previously in file /tmp/.tmpu0X9jI/hugr-llvm/src/custom.rs:50 method emitter of trait CodegenExtension, previously in file /tmp/.tmpu0X9jI/hugr-llvm/src/custom.rs:58 method load_constant of trait CodegenExtension, previously in file /tmp/.tmpu0X9jI/hugr-llvm/src/custom.rs:68 --- failure trait_missing: pub trait removed or renamed --- Description: A publicly-visible trait cannot be imported by its prior path. A `pub use` may have been removed, or the trait itself may have been renamed or removed entirely. ref: https://doc.rust-lang.org/cargo/reference/semver.html#item-remove impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.36.0/src/lints/trait_missing.ron Failed in: trait hugr_llvm::emit::EmitOp, previously in file /tmp/.tmpu0X9jI/hugr-llvm/src/emit.rs:45 trait hugr_llvm::custom::prelude::PreludeCodegen, previously in file /tmp/.tmpu0X9jI/hugr-llvm/src/custom/prelude.rs:52 trait hugr_llvm::fat::FatExt, previously in file /tmp/.tmpu0X9jI/hugr-llvm/src/fat.rs:313 --- failure trait_no_longer_object_safe: trait no longer object safe --- Description: Trait is no longer object safe, which breaks `dyn Trait` usage. ref: https://doc.rust-lang.org/stable/reference/items/traits.html#object-safety impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.36.0/src/lints/trait_no_longer_object_safe.ron Failed in: trait CodegenExtension in file /tmp/.tmpGjujM0/hugr-llvm/src/custom.rs:38 ``` <details><summary>Changelog</summary> <blockquote> ## [0.6.0](v0.5.1...v0.6.0) - 2024-10-21 ### Bug Fixes - Conversion operations having poison results ([#131](#131)) ### New Features - [**breaking**] Allow extension callbacks to have non-`'static` lifetimes ([#128](#128)) - [**breaking**] Support `tket2.rotation.from_halfturns_unchecked` ([#133](#133)) ### Refactor - [**breaking**] remove trait emit op ([#104](#104)) - [**breaking**] rework extensions interface ([#119](#119)) - [**breaking**] move packaged extensions from `crate::custom` to `crate::extension` ([#126](#126)) </blockquote> </details> --- This PR was generated with [release-plz](https://github.com/MarcoIeni/release-plz/).

doug-q added 3 commits October 20, 2024 10:50

fix: Conversion operations haveing poison results.

118e779

test: add test for roundtripping signed ints.

176d98f

test: Add tests for exact float <-> int roundtrips

d239a64

doug-q requested a review from a team as a code owner October 20, 2024 10:44

doug-q requested a review from cqc-alec October 20, 2024 10:44

doug-q added 2 commits October 20, 2024 11:45

tidy

e17b7a5

fmt

8e285f8

doug-q commented Oct 21, 2024

View reviewed changes

doug-q mentioned this pull request Oct 21, 2024

chore: update to hugr 0.13 and tket2 0.6 #130

Closed

ss2165 mentioned this pull request Oct 21, 2024

chore: update devenv #134

Merged

Merge branch 'main' into doug/fix-poison-conversions

3c20cd3

cqc-alec approved these changes Oct 21, 2024

View reviewed changes

doug-q added this pull request to the merge queue Oct 21, 2024

Merged via the queue into main with commit 22d1d0f Oct 21, 2024
10 checks passed

doug-q deleted the doug/fix-poison-conversions branch October 21, 2024 10:55

hugrbot mentioned this pull request Oct 21, 2024

chore: release v0.6.0 #114

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Conversion operations having poison results #131

fix: Conversion operations having poison results #131

doug-q commented Oct 20, 2024

doug-q commented Oct 21, 2024

doug-q Oct 21, 2024

codecov-commenter commented Oct 21, 2024

cqc-alec Oct 21, 2024

doug-q Oct 21, 2024

cqc-alec Oct 21, 2024

doug-q Oct 21, 2024

cqc-alec Oct 21, 2024

fix: Conversion operations having poison results #131

fix: Conversion operations having poison results #131

Conversation

doug-q commented Oct 20, 2024

doug-q commented Oct 21, 2024

doug-q Oct 21, 2024

Choose a reason for hiding this comment

codecov-commenter commented Oct 21, 2024

Codecov Report

cqc-alec Oct 21, 2024

Choose a reason for hiding this comment

doug-q Oct 21, 2024

Choose a reason for hiding this comment

cqc-alec Oct 21, 2024

Choose a reason for hiding this comment

doug-q Oct 21, 2024

Choose a reason for hiding this comment

cqc-alec Oct 21, 2024

Choose a reason for hiding this comment