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

Conditional formats #362

Merged
merged 5 commits into from
Jun 1, 2022
Merged

Conditional formats #362

merged 5 commits into from
Jun 1, 2022

Conversation

wezm
Copy link
Contributor

@wezm wezm commented Jun 1, 2022

Introduce support for conditional formats. Fixes #339

This allows values to be verified against a condition. E.g.

{ sfnt_version <- { version <- u32be | (u32_eq version 0xffff) } }

If the predicate succeeds then the format is read as usual. If it fails this currently results in an error being returned from read_format, which in turn is presented as a panic/internal compiler error. I'm opening this PR to get the work reviewed so far and also to discuss how that fail case should be handled. I also need to update the reference and add some more tests when everything is locked down.

@brendanzab
Copy link
Member

Looks great! We're missing cases in is_equal in core::semantics, and unify in surface::elaboration::unification, but things seem to be in order! 👍

@wezm
Copy link
Contributor Author

wezm commented Jun 1, 2022

We're missing cases in is_equal in core::semantics, and unify in surface::elaboration::unification

Ahh thank you. Would be ideal if these could be compilation errors but a quick look suggests that's not super easy without lots of verbosity.

@brendanzab
Copy link
Member

Yeah, it's a bit frustrating that we don't get any errors there :(

@wezm
Copy link
Contributor Author

wezm commented Jun 1, 2022

Yeah, it's a bit frustrating that we don't get any errors there :(

What do you think of this as a workaround for that?

    #[test]
    fn value_has_unify_and_is_equal_impls() {
        let value = Arc::new(Value::ConstLit(Const::Bool(false)));

        // This test exists in order to cause a test failure when `Value` is changed. If this test
        // has failed and you have added a new variant to Value it is a prompt to ensure that
        // variant is handled in:
        //
        // - surface::elaboration::Context::unify
        // - core::semantics::is_equal
        //
        // NOTE: Only update the match below when you've updated the above functions.
        match value.as_ref() {
            Value::Stuck(_, _) => {}
            Value::Universe => {}
            Value::FunType(_, _, _) => {}
            Value::FunLit(_, _) => {}
            Value::RecordType(_, _) => {}
            Value::RecordLit(_, _) => {}
            Value::ArrayLit(_) => {}
            Value::FormatRecord(_, _) => {}
            Value::FormatCond(_, _, _) => {}
            Value::FormatOverlap(_, _) => {}
            Value::ConstLit(_) => {}
        }
    }

@brendanzab
Copy link
Member

Oooh, I like that. Clever idea!

doc/reference.md Outdated
Comment on lines 292 to 294
The [representation](#format-representations) of a conditional format is the same
as the format without the condition when the condition evaluates to `true`. If
the condition is `false` then it's equivalent to the [fail format](#fail-format).
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure this is entirely accurate… this kind of implies something like Repr format | Void? Where as we want something more like:

format Repr format
{ x <- format | cond } Repr format

Not sure how to express this better in words, but it might be more accurate to say that “the representation of a conditional format is the same as the representation of the format guarded by the predicate”. It's a bit clunky to describe in words as we don't really have a name for what we're talking about. 😕

Copy link
Contributor Author

Choose a reason for hiding this comment

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

“the representation of a conditional format is the same as the representation of the format guarded by the predicate”

Happy to change it to this. It's basically what I was was trying to say 😅

@wezm wezm force-pushed the conditional-formats branch 2 times, most recently from a0b0e98 to 9d9da01 Compare June 1, 2022 06:20
Copy link
Member

@brendanzab brendanzab left a comment

Choose a reason for hiding this comment

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

Looks good! 👍

@wezm wezm merged commit abf2a67 into main Jun 1, 2022
@wezm wezm deleted the conditional-formats branch June 1, 2022 23:34
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.

Conditional formats
2 participants