Skip to content

RFC 53 Lean model and proofs#530

Merged
shaobo-he-aws merged 13 commits intomainfrom
feature/shaobo/rfc53-one-schema
Feb 10, 2025
Merged

RFC 53 Lean model and proofs#530
shaobo-he-aws merged 13 commits intomainfrom
feature/shaobo/rfc53-one-schema

Conversation

@shaobo-he-aws
Copy link
Contributor

Issue #, if available:

Description of changes:

Signed-off-by: Shaobo He <shaobohe@amazon.com>
Signed-off-by: Shaobo He <shaobohe@amazon.com>
@shaobo-he-aws shaobo-he-aws mentioned this pull request Feb 7, 2025
@shaobo-he-aws shaobo-he-aws marked this pull request as ready for review February 7, 2025 19:45
Signed-off-by: Shaobo He <shaobohe@amazon.com>
Signed-off-by: Shaobo He <shaobohe@amazon.com>
Signed-off-by: Shaobo He <shaobohe@amazon.com>
Copy link
Contributor

@emina emina left a comment

Choose a reason for hiding this comment

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

Nice PR! I like this approach.

Left small comments / suggestions on the model.

Signed-off-by: Shaobo He <shaobohe@amazon.com>
Signed-off-by: Shaobo He <shaobohe@amazon.com>
Signed-off-by: Shaobo He <shaobohe@amazon.com>
Signed-off-by: Shaobo He <shaobohe@amazon.com>
Signed-off-by: Shaobo He <shaobohe@amazon.com>
Signed-off-by: Shaobo He <shaobohe@amazon.com>
@emina
Copy link
Contributor

emina commented Feb 8, 2025

Thinking about this more, we need to change the definition of InstanceOfEntityType and fix the upstream definitions and proofs accordingly.

In particular, the definition should be this:


def InstanceOfEntityType (e : EntityUID) (ety: EntityType) (env : Environment) : Prop :=
  e.ty = ety && (env.ets.isValidEntityUID e.ty  ∨ env.acts.contains e)

Without this change, InstanceOfType admits invalid enum literals. And this means that our type soundness theorem becomes too weak. It says that the evaluator may produce invalid entity literals even for type-correct expressions. The type checker code actually prevents this, but it also needs to be reflected in the definition of InstanceOfType and the soundness theorem.

Note that we have already made the corresponding change in RequestEntityValidator and instanceOfEntityType.

@emina
Copy link
Contributor

emina commented Feb 8, 2025

Actually, I now realize that the definition of InstanceOfEntityType has always been too weak (!).

Note that it admits entity literals of arbitrary entity types that aren’t declared in the schema.

The actual type checking algorithm doesn’t admit this.

So, even before the addition of entity enums, this definition should have taken the environment as input and looked like this instead:

def InstanceOfEntityType (e : EntityUID) (ety: EntityType) (env : Environment) : Prop :=
  e.ty = ety && (env.ets.contains e.ty  ∨ env.acts.contains e)

@shaobo-he-aws
Copy link
Contributor Author

shaobo-he-aws commented Feb 8, 2025

So, even before the addition of entity enums, this definition should have taken the environment as input and looked like this instead:

def InstanceOfEntityType (e : EntityUID) (ety: EntityType) (env : Environment) : Prop :=
  e.ty = ety && (env.ets.contains e.ty  ∨ env.acts.contains e)

To make the scope of this PR limited to RFC 53, I propose that we fix the definition first in a separate PR and rebase this PR based on it. I'll work on the fix first.

In fact, env.ets.isValidEntityUID e.ty implies env.ets.contains e.ty so we might as well fix the definition in this PR.

@emina
Copy link
Contributor

emina commented Feb 8, 2025

In fact, env.ets.isValidEntityUID e.ty implies env.ets.contains e.ty so we might as well fix the definition in this PR.

I'll approve this PR as-is, in case you find it's easier to fix this in a separate PR.

Copy link
Contributor

@cdisselkoen cdisselkoen 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, just some small nits

Signed-off-by: Shaobo He <shaobohe@amazon.com>
@shaobo-he-aws
Copy link
Contributor Author

shaobo-he-aws commented Feb 10, 2025

Will merge this PR first and then address the weakness of InstanceOfEntityType later in a separate PR because it will be quite involved.

@shaobo-he-aws shaobo-he-aws merged commit ead3c16 into main Feb 10, 2025
6 checks passed
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.

3 participants