Skip to content

Simplified strict validation#111

Merged
john-h-kastner-aws merged 10 commits intomainfrom
feature/jkastner/simpl_strict_validation
Sep 8, 2023
Merged

Simplified strict validation#111
john-h-kastner-aws merged 10 commits intomainfrom
feature/jkastner/simpl_strict_validation

Conversation

@john-h-kastner-aws
Copy link
Contributor

Issue #, if available:

cedar-policy/rfcs#19
cedar-policy/cedar#282

Description of changes:

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@john-h-kastner-aws john-h-kastner-aws changed the title Feature/jkastner/simpl strict validation Simplified strict validation Aug 18, 2023
Copy link
Contributor

@khieta khieta left a comment

Choose a reason for hiding this comment

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

Overall looks good. I left a few small comments.

Copy link
Contributor

@khieta khieta left a comment

Choose a reason for hiding this comment

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

A couple more quick comments.

function lubEntity(lub1: EntityLUB, lub2: EntityLUB): EntityLUB {
lub1.union(lub2)
function lubEntity(lub1: EntityLUB, lub2: EntityLUB, m: ValidationMode): Result<EntityLUB> {
if m.isStrict() && lub1 != lub2
Copy link
Contributor

Choose a reason for hiding this comment

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

In strict mode, do you also want to check that neither are AnyEntity?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For strict mode this already gives E </: AnyEntity for E where E != AnyEntity. I could change it so that AnyEntity </: AnyEntity, but then subtyping isn't reflexive (which is strange). I don't think it really matters one way or the other since AnyEntity shouldn't be constructed by the strict typechecker as along as it doesn't show up in the request environment anywhere.

@john-h-kastner-aws john-h-kastner-aws force-pushed the feature/jkastner/simpl_strict_validation branch from 61f4208 to fd1a16c Compare September 8, 2023 17:56

// The ValidationMode determines whether to use permissive or strict typechecking
datatype ValidationMode = Permissive | Strict {
predicate isStrict() {
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if it makes sense to use the Strict? method Dafny automatically generates instead.

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 you're right

attrsTag.OpenAttributes?
}

predicate isStrictType() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe -> isStrict?

Entity(lub: EntityLUB) |
Extension(Name)
{
predicate isStrictType() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above.

{
assert subty(getType(ei2s[i],effs), eltType);
SubtyTrans(getType(ei2s[i],effs), eltType, Type.Entity(AnyEntity));
assert subty(getType(ei2s[i],effs), eltType,ValidationMode.Permissive);
Copy link
Contributor

Choose a reason for hiding this comment

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

[nit] noticed we aren't consistent about spaces after commas. Unfortunately, it doesn't seem like formatting in vscode handles this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

dafny format doesn't either unfortunately.

@john-h-kastner-aws john-h-kastner-aws merged commit fa6d9aa into main Sep 8, 2023
@khieta khieta deleted the feature/jkastner/simpl_strict_validation branch November 16, 2023 17:56
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.

4 participants