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

More type checking #99

Open
wants to merge 12 commits into
base: main
Choose a base branch
from
Open

More type checking #99

wants to merge 12 commits into from

Conversation

chip-so
Copy link
Contributor

@chip-so chip-so commented Feb 13, 2025

Closes #14

@chip-so
Copy link
Contributor Author

chip-so commented Feb 13, 2025

I still have some tests to write, but please take a look.

@chip-so chip-so marked this pull request as ready for review February 18, 2025 23:56
"Cannot publish `{ot}`, must be a command struct"
))))
}
_ => {}

Choose a reason for hiding this comment

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

Does this allow for something like?

    action foo() {
        publish None
    }

Copy link
Contributor Author

@chip-so chip-so Feb 20, 2025

Choose a reason for hiding this comment

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

Yes. None is the other hole in the type system. It is an indeterminate type and thus is allowed anywhere.

Choose a reason for hiding this comment

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

It is an indeterminate type and thus is allowed anywhere

Should it be allowed here if we're expecting only Command structs to be published?

I could be wrong but I don't think any valid programs would be rejected if expressions with an indeterminate type weren't allowed.

I thought about something like

action foo() {
    // Assume Foo is a Command struct
    let cmd_struct = Foo {}
    let bytes = serialize(cmd_struct)
    let deserialized_foo = deserialize(bytes) // indeterminate type
    
    publish deserialized_foo
}

but using serialize/deserialize in an action results in a compilation error

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should it be allowed here if we're expecting only Command structs to be published?

I suppose not. Could probably enforce that on other things too (like the condition in if only accepts bools, so an indeterminate optional or struct is right out).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So I don't think we can restrict this generally because indeterminate types propagate through some operators. e.g. unwrap, is, and the arms of an if expression. So you can make reasonable enough code like this:

action maybe_publish(a int) {
    let z = if a > 3 {
        None
    } else {
        Some TestCommand{
            x: a,
        }
    }

    check z is Some
    let c = unwrap z

    publish c
}

And that indeterminacy propagates all the way down through the if, is, check, and unwrap. By the rules of the language, we know c is perfectly valid, but the type checker doesn't know it.

The obvious way to fix this is to unify the None with the Some in the if branches, but to do that we need to know that the None is an "optional of unknown" instead of indeterminate. That's an enhancement to the type checker I want to do eventually, so I think we should leave this for later.

apetkov-so
apetkov-so previously approved these changes Feb 20, 2025
schema_key.identifier, schema_key.field_type
))));
};
// Type checking handled in compile_fact_literal() now
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to call compile_fact_literal()? I'm trying to remember why these checks had to be done here.

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 think it was just a convenient place for them. I did check, and every place where verify_fact_against_schema() was called is followed immediately by compile_fact_literal() (which does suggest some refactoring is in order).

"Cannot publish `{ot}`, must be a command struct"
))))
}
_ => {}
Copy link
Contributor

Choose a reason for hiding this comment

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

What do we do in this case?

Copy link
Contributor Author

@chip-so chip-so Feb 21, 2025

Choose a reason for hiding this comment

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

In this case we can make no determination about whether the type is correct or not (it matches only Typeish::Indeterminate), so it is allowed. But as Steve quite rightly points out above, there is no case where we produce an indeterminate value that could be valid here anyway. So I will change this to be disallowed as well and see what breaks.

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.

Check types for more things
3 participants