fix: Add some validation for const nodes #1222

cqc-alec · 2024-06-24T12:52:57Z

Fixes #1185. We add a test for #1091, but the fix unclear now, see #1225.

codecov · 2024-06-24T12:58:49Z

Codecov Report

Attention: Patch coverage is 72.41379% with 16 lines in your changes missing coverage. Please review.

Project coverage is 86.99%. Comparing base (4ef4826) to head (08df26a).

Files	Patch %	Lines
hugr-core/src/hugr.rs	69.44%	11 Missing ⚠️
hugr-core/src/ops/constant.rs	72.22%	3 Missing and 2 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1222      +/-   ##
==========================================
- Coverage   87.03%   86.99%   -0.05%     
==========================================
  Files         100      100              
  Lines       18918    18975      +57     
  Branches    16935    16992      +57     
==========================================
+ Hits        16466    16508      +42     
- Misses       1676     1689      +13     
- Partials      776      778       +2

Flag	Coverage Δ
python	`91.42% <ø> (ø)`
rust	`86.48% <72.41%> (-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.

doug-q

This test for floats is too specific, I think that generalising the code will mean that the fix for bad CustomConst will need to come from tweaking error handling around CustomSerialised.

doug-q · 2024-06-24T13:18:36Z

hugr-core/src/hugr/validate.rs

@@ -210,6 +212,35 @@ impl<'a, 'b> ValidationContext<'a, 'b> {
        // Thirdly that the node has correct children
        self.validate_children(node, op_type)?;

+        // OpType-specific checks.
+        // TODO Maybe we should delegate these checks to the OpTypes themselves.


Maybe, but I can see why you don't. ValidateOp is not set up for op-specific logic. I think it's fine here for now, but I do think you should extract it into a function. i.e.

if let OpType::Const(c) = op_type { match c.validate() }

doug-q · 2024-06-24T13:29:32Z

hugr-core/src/hugr/validate.rs

+            match c.get_type().as_type_enum() {
+                TypeEnum::Sum(SumType::Unit { size: _ }) => {
+                    if let Value::Sum {
+                        tag: _,
+                        values,
+                        sum_type: _,
+                    } = c.value()
+                    {
+                        if !values.is_empty() {
+                            return Err(ValidationError::UnitSumWithValues {});
+                        }
+                    }
+                }
+                TypeEnum::Extension(custom_type) => {
+                    if custom_type.name() == "float64" {
+                        if let Value::Extension { e } = c.value() {
+                            if e.value().type_id() != TypeId::of::<ConstF64>() {
+                                return Err(ValidationError::WrongExtensionValueType {});
+                            }
+                        }
+                    }
+                }
+                _ => {}
+            }


I think this is too specific to the bugs. We need to check all sum values have the right type, and we need logic that will work for all CustomConsts.

A start is: (the body of Value::validate):

Suggested change

match c.get_type().as_type_enum() {

TypeEnum::Sum(SumType::Unit { size: _ }) => {

if let Value::Sum {

tag: _,

values,

sum_type: _,

} = c.value()

{

if !values.is_empty() {

return Err(ValidationError::UnitSumWithValues {});

}

}

}

TypeEnum::Extension(custom_type) => {

if custom_type.name() == "float64" {

if let Value::Extension { e } = c.value() {

if e.value().type_id() != TypeId::of::<ConstF64>() {

return Err(ValidationError::WrongExtensionValueType {});

}

}

}

}

_ => {}

}

match self {

Self::Extension { e } => e.value().validate(),

Self::Sum { tag, typ, values } => typ.check_type(values),

Self::Function { .. } => Ok(()) // we don't check inside Function Vlaues

}

Thanks! So, just to clarify, is your suggestion to:

create a new validate() method for Values, and call that (when appropriate) from validate_node();

in Value::validate(), for the case of Extension values, delegate to CustomConst::validate();

add overloads for CustomConst::validate() for specific types such as ConstF64?

Yes. ConstF64 does need a validate, which it doesn't have, but that is a different issue: It can't check serialisation, because ConstF64 only holds a f64, but it should check that it's not NAN or infinite.

Yes indeed, and adding a validate() to ConstF64 wouldn't catch the issue here, which is that we haven't got a ConstF64 at all...

doug-q · 2024-06-24T13:33:32Z

hugr-core/src/hugr.rs

+        ))
+        .unwrap();
+        assert!(
+            hugr.update_validate(&PRELUDE_REGISTRY).is_err(),


Needs to include float extensions here and below I think?. Maybe it's fine because CustomConst don't work through extensions.

…t_ops

doug-q · 2024-06-25T13:01:32Z

@cqc-alec can you review? I've punted on #1091 and have created #1225 to explain the difficulty.

cqc-alec · 2024-06-25T13:08:01Z

hugr-core/src/hugr/validate.rs

@@ -214,6 +215,12 @@ impl<'a, 'b> ValidationContext<'a, 'b> {
        // Thirdly that the node has correct children
        self.validate_children(node, op_type)?;

+        // OpType-specific checks.
+        // TODO Maybe we should delegate these checks to the OpTypes themselves.


Perhaps this TODO can be removed now?

I think it should remain, it does feel like this should call into a trait.

cqc-alec · 2024-06-25T13:08:18Z

hugr-core/src/hugr/validate.rs

@@ -759,6 +766,9 @@ pub enum ValidationError {
    /// [Opaque]: crate::ops::CustomOp::Opaque
    #[error(transparent)]
    CustomOpError(#[from] CustomOpError),
+    /// TODO


Add docstring?

Awkward, will post a PR to fixup this

cqc-alec · 2024-06-25T13:19:03Z

@cqc-alec can you review? I've punted on #1091 and have created #1225 to explain the difficulty.

LGTM though it seems I can't "approve" it. I haven't quite figured out where the check for empty value lists in unit sums is happening now...?

doug-q · 2024-06-25T13:28:06Z

@cqc-alec can you review? I've punted on #1091 and have created #1225 to explain the difficulty.

LGTM though it seems I can't "approve" it. I haven't quite figured out where the check for empty value lists in unit sums is happening now...?

In SumType::check_type in types/check.rs

cqc-alec · 2024-06-25T13:39:42Z

I haven't quite figured out where the check for empty value lists in unit sums is happening now...?

In SumType::check_type in types/check.rs

I got that far, just couldn't see where the check was, but I see it now, thanks!

## 🤖 New release * `hugr`: 0.5.1 -> 0.6.0 * `hugr-core`: 0.2.0 -> 0.3.0 * `hugr-passes`: 0.2.0 -> 0.3.0 * `hugr-cli`: 0.1.1 -> 0.1.2 <details><summary>Changelog</summary> ## `hugr` <blockquote> ## 0.6.0 (2024-06-28) ### Bug Fixes - SimpleReplacement panic on multiports ([#1191](#1191)) - Add some validation for const nodes ([#1222](#1222)) - Cfg not validating entry/exit types ([#1229](#1229)) - `extract_hugr` not removing root node ports ([#1239](#1239)) ### Documentation - Fix documentation of `ValidationError::ConstTypeError` ([#1227](#1227)) ### Features - CircuitBuilder::add_constant ([#1168](#1168)) - [**breaking**] Make the rewrite errors more useful ([#1174](#1174)) - [**breaking**] Validate Extensions using hierarchy, ignore input_extensions, RIP inference ([#1142](#1142)) - [**breaking**] Infer extension deltas for Case, Cfg, Conditional, DataflowBlock, Dfg, TailLoop ([#1195](#1195)) - Helper functions for requesting inference, use with builder in tests ([#1219](#1219)) ### Refactor - [**breaking**] FunctionBuilder takes impl Into<PolyFuncType> ([#1220](#1220)) - [**breaking**] Remove NodeType and input_extensions ([#1183](#1183)) </blockquote> ## `hugr-core` <blockquote> ## 0.3.0 (2024-06-28) ### Bug Fixes - SimpleReplacement panic on multiports ([#1191](#1191)) - Add some validation for const nodes ([#1222](#1222)) - Cfg not validating entry/exit types ([#1229](#1229)) - `extract_hugr` not removing root node ports ([#1239](#1239)) ### Documentation - Fix documentation of `ValidationError::ConstTypeError` ([#1227](#1227)) ### Features - CircuitBuilder::add_constant ([#1168](#1168)) - [**breaking**] Make the rewrite errors more useful ([#1174](#1174)) - [**breaking**] Validate Extensions using hierarchy, ignore input_extensions, RIP inference ([#1142](#1142)) - [**breaking**] Infer extension deltas for Case, Cfg, Conditional, DataflowBlock, Dfg, TailLoop ([#1195](#1195)) - Helper functions for requesting inference, use with builder in tests ([#1219](#1219)) ### Refactor - [**breaking**] Remove NodeType and input_extensions ([#1183](#1183)) - [**breaking**] FunctionBuilder takes impl Into<PolyFuncType> ([#1220](#1220)) </blockquote> ## `hugr-passes` <blockquote> ## 0.3.0 (2024-06-28) ### Features - [**breaking**] Validate Extensions using hierarchy, ignore input_extensions, RIP inference ([#1142](#1142)) - Helper functions for requesting inference, use with builder in tests ([#1219](#1219)) </blockquote> ## `hugr-cli` <blockquote> ## 0.1.1 (2024-06-07) ### Features - Reexport `clap::Parser` and `clap_verbosity_flag::Level` from hugr_cli ([#1146](#1146)) ### Refactor - Move binary to hugr-cli ([#1134](#1134)) </blockquote> </details> --- This PR was generated with [release-plz](https://github.com/MarcoIeni/release-plz/). --------- Co-authored-by: Douglas Wilson <[email protected]>

🤖 I have created a release *beep* *boop* --- ## [0.3.0](hugr-py-v0.2.1...hugr-py-v0.3.0) (2024-07-03) ### ⚠ BREAKING CHANGES * * `add_child_op`(`_with_parent`), etc., gone; use `add_child_node`(`_with_parent`) with an (impl Into-)OpType. * `get_nodetype` gone - use `get_optype`. * `NodeType` gone - use `OpType` directly. * Various (Into<)Option<ExtensionSet> params removed from builder methods especially {cfg_,dfg_}builder. * `input_extensions` removed from serialization schema. * the Signature class is gone, but it's not clear how or why you might have been using it... * TailLoop node and associated builder functions now require specifying an ExtensionSet; extension/validate.rs deleted; some changes to Hugrs validated/rejected when the `extension_inference` feature flag is turned on * Type::validate takes extra bool (allow_rowvars); renamed {FunctionType, PolyFuncType}::(validate=>validate_var_len). ### Features * Allow "Row Variables" declared as List<Type> ([#804](#804)) ([3ea4834](3ea4834)) * **hugr-py:** add builders for Conditional and TailLoop ([#1210](#1210)) ([43569a4](43569a4)) * **hugr-py:** add CallIndirect, LoadFunction, Lift, Alias ([#1218](#1218)) ([db09193](db09193)), closes [#1213](#1213) * **hugr-py:** add values and constants ([#1203](#1203)) ([f7ea178](f7ea178)), closes [#1202](#1202) * **hugr-py:** automatically add state order edges for inter-graph edges ([#1165](#1165)) ([5da06e1](5da06e1)) * **hugr-py:** builder for function definition/declaration and call ([#1212](#1212)) ([af062ea](af062ea)) * **hugr-py:** builder ops separate from serialised ops ([#1140](#1140)) ([342eda3](342eda3)) * **hugr-py:** CFG builder ([#1192](#1192)) ([c5ea47f](c5ea47f)), closes [#1188](#1188) * **hugr-py:** define type hierarchy separate from serialized ([#1176](#1176)) ([10f4c42](10f4c42)) * **hugr-py:** only require input type annotations when building ([#1199](#1199)) ([2bb079f](2bb079f)) * **hugr-py:** python hugr builder ([#1098](#1098)) ([23408b5](23408b5)) * **hugr-py:** store children in node weight ([#1160](#1160)) ([1cdaeed](1cdaeed)), closes [#1159](#1159) * **hugr-py:** ToNode interface to treat builders as nodes ([#1193](#1193)) ([1da33e6](1da33e6)) * Validate Extensions using hierarchy, ignore input_extensions, RIP inference ([#1142](#1142)) ([8bec8e9](8bec8e9)) ### Bug Fixes * Add some validation for const nodes ([#1222](#1222)) ([c05edd3](c05edd3)) * **hugr-py:** more ruff lints + fix some typos ([#1246](#1246)) ([f158384](f158384)) * **py:** get rid of pydantic config deprecation warnings ([#1084](#1084)) ([52fcb9d](52fcb9d)) ### Documentation * **hugr-py:** add docs link to README ([#1259](#1259)) ([d2a9148](d2a9148)) * **hugr-py:** build and publish docs ([#1253](#1253)) ([902fc14](902fc14)) * **hugr-py:** docstrings for builder ([#1231](#1231)) ([3e4ac18](3e4ac18)) ### Code Refactoring * Remove "Signature" from hugr-py ([#1186](#1186)) ([65718f7](65718f7)) * Remove NodeType and input_extensions ([#1183](#1183)) ([ea5213d](ea5213d)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). --------- Co-authored-by: hugrbot <[email protected]> Co-authored-by: Seyon Sivarajah <[email protected]>

cqc-alec added 4 commits June 24, 2024 13:48

Add test cases (two failing).

9f203e1

Check validity of SumType::Unit nodes.

3c0b2a1

Check validity of "float64" values.

e207dd1

Move resources directory up a level and add symlink.

800a6e7

cqc-alec requested a review from doug-q June 24, 2024 12:58

cqc-alec marked this pull request as ready for review June 24, 2024 12:58

cqc-alec requested a review from a team as a code owner June 24, 2024 12:58

doug-q requested changes Jun 24, 2024

View reviewed changes

doug-q added 3 commits June 25, 2024 13:48

validate const nodes. fix unearthed bug in const_fold::test::test_lis…

0fee7e9

…t_ops

Merge branch 'main' of gh:CQCL-DEV/hugr into ae/constvals

6b9427e

fix builder test which was constructing an invalid value

08df26a

cqc-alec commented Jun 25, 2024

View reviewed changes

doug-q enabled auto-merge June 25, 2024 13:29

doug-q approved these changes Jun 25, 2024

View reviewed changes

doug-q added this pull request to the merge queue Jun 25, 2024

Merged via the queue into main with commit c05edd3 Jun 25, 2024
21 of 22 checks passed

doug-q deleted the ae/constvals branch June 25, 2024 13:31

This was referenced Jun 25, 2024

chore(main): release hugr-py 0.3.0 #1092

Merged

chore: release #1181

Closed

This was referenced Jun 28, 2024

chore: release #1233

Closed

chore: release #1240

Merged

hugrbot mentioned this pull request Jul 1, 2024

chore: release #1248

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Add some validation for const nodes #1222

fix: Add some validation for const nodes #1222

cqc-alec commented Jun 24, 2024 •

edited by doug-q

Loading

codecov bot commented Jun 24, 2024 •

edited

Loading

doug-q left a comment

doug-q Jun 24, 2024

doug-q Jun 24, 2024

cqc-alec Jun 25, 2024

doug-q Jun 25, 2024

cqc-alec Jun 25, 2024

doug-q Jun 24, 2024

doug-q commented Jun 25, 2024

cqc-alec Jun 25, 2024

doug-q Jun 25, 2024

cqc-alec Jun 25, 2024

doug-q Jun 25, 2024

cqc-alec commented Jun 25, 2024

doug-q commented Jun 25, 2024

cqc-alec commented Jun 25, 2024

fix: Add some validation for const nodes #1222

fix: Add some validation for const nodes #1222

Conversation

cqc-alec commented Jun 24, 2024 • edited by doug-q Loading

codecov bot commented Jun 24, 2024 • edited Loading

Codecov Report

doug-q left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

doug-q commented Jun 25, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cqc-alec commented Jun 25, 2024

doug-q commented Jun 25, 2024

cqc-alec commented Jun 25, 2024

cqc-alec commented Jun 24, 2024 •

edited by doug-q

Loading

codecov bot commented Jun 24, 2024 •

edited

Loading