Skip to content

feat!: Innards of non-empty holes should check against hole type#931

Merged
brprice merged 2 commits intomainfrom
brprice/nehole-chk
May 22, 2023
Merged

feat!: Innards of non-empty holes should check against hole type#931
brprice merged 2 commits intomainfrom
brprice/nehole-chk

Conversation

@brprice
Copy link
Copy Markdown
Contributor

@brprice brprice commented Apr 6, 2023

Currently they must synthesise some type (and we don't care what), but there is no particular reason for this that I can recall, other than following Hazel. I think it may be nicer UX if we change this.

In particular, with checkable saturated constructors we would then be able to write a constructor directly inside a hole, without having to annotate it. Even without that, we could write a lambda directly inside a hole (currently it would need an annotation): {? λx.t[x] ?} would be valid, and synthesise a hole exactly when we have that x : ? ⊢ ? ∋ t[x].

Essentially, whenever we now write {? t : ? ?}, we could drop the annotation. Whenever we currently have a more-specific annotation (currently this is only possible when a human explicitly writes it, I think), then we could still write it exactly the same as before (e.g. {? λx.t[x] : Nat -> Bool ?} would be fine exactly when x : Nat ⊢ Bool ∋ t[x]).

@georgefst
Copy link
Copy Markdown
Contributor

This sounds like a good idea. Do you see any possible downsides?

@brprice
Copy link
Copy Markdown
Contributor Author

brprice commented Apr 6, 2023

This sounds like a good idea. Do you see any possible downsides?

Other than time required to implement, no I don't. However, I have not thought about how this would interact with hole removal (either automatically via smartholes, or manually via an action). I would also not be totally surprised if some unexpected problems arise during implementation.

@georgefst
Copy link
Copy Markdown
Contributor

Okay. Maybe let's wait and see how often this comes up as an issue when we start using the app with saturated constructors.

brprice added a commit that referenced this pull request Apr 6, 2023
…ds we should change this; we have not caught it before as we don't PBT progactions -- @georgefst: (how) will this interact with your typedefs-in-openapi work?
@brprice
Copy link
Copy Markdown
Contributor Author

brprice commented Apr 6, 2023

There is an interaction between this and #932: they both care about directionality of holes and constructor arguments. When we come back to this, we need to update the fix and tests for #932 EDIT: this has been done

Comment thread primer/src/Primer/Core.hs Outdated
-- I have modified the original Hazel mechanised metatheory
-- (https://github.com/hazelgrove/agda-popl17) and changed it to have this rule
-- (and a few other knock-on changes).
-- See https://github.com/brprice/hazel-experiments
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Either this repo doesn't exist, or I don't have read permissions.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

FTR

@dhess
Copy link
Copy Markdown
Member

dhess commented Apr 10, 2023

I don't understand this sentence in the OP:

Whenever we currently have a more-specific annotation (currently, only when a human writes it, I think), when we could still write it (e.g. {? λx.t[x] : Nat -> Bool ?} would be fine exactly when x : Nat ⊢ Bool ∋ t[x]).

@dhess
Copy link
Copy Markdown
Member

dhess commented Apr 10, 2023

I think this sounds like a good idea. It seems very likely that students will be spending a lot of time editing misfit expressions [1] , and whatever we can do to make that workflow less confusing, the better. Automatically-inserted annotations will probably not be very intuitive, especially if they're unlikely to occur in other situations.

[1] n.b.: I'm adopting our new terminology here, misfit expression rather than what we previously called non-empty hole

@brprice brprice force-pushed the brprice/nehole-chk branch from 0388897 to ddc0c75 Compare April 26, 2023 20:54
@brprice brprice force-pushed the brprice/nehole-chk branch from ccd6444 to 1a9cece Compare May 11, 2023 20:05
@brprice brprice changed the title [WIP/RFC]: Innards of non-empty holes should check against hole type feat: Innards of non-empty holes should check against hole type May 11, 2023
@brprice brprice changed the title feat: Innards of non-empty holes should check against hole type feat!: Innards of non-empty holes should check against hole type May 11, 2023
@brprice brprice force-pushed the brprice/nehole-chk branch from 1a9cece to 7bb6ab1 Compare May 11, 2023 20:09
@brprice brprice changed the base branch from main to brprice/letrec-typing May 11, 2023 20:11
Comment on lines 478 to 489
, ConstructVar $ LocalVarRef "x"
, ConstructAnn
, Move Child1
, ConstructCase
, Move (Branch cTrue)
, constructSaturatedCon cZero
]
( ann
( lam "x" $
hole $
ann
( case_
(lvar "x")
[branch cTrue [] (con0 cZero), branch cFalse [] emptyHole]
)
tEmptyHole
case_
(lvar "x")
[branch cTrue [] (con0 cZero), branch cFalse [] emptyHole]
)
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Note how we no longer have to annotate this case inside the hole.

@brprice
Copy link
Copy Markdown
Contributor Author

brprice commented May 11, 2023

Note for reviewers: This work is based off #979 and the first commit is, as stated in its commit message, cherrypicked from #976. This first commit will be rebased away once #976 and #979 land.

@brprice brprice marked this pull request as ready for review May 11, 2023 20:19
@brprice brprice requested a review from a team May 11, 2023 20:19
@dhess
Copy link
Copy Markdown
Member

dhess commented May 12, 2023

I'm happy with this decision, and thanks for the well-motivated discussion in one of the code comments in the 2nd commit. However, I'll leave the code review to @georgefst.

@brprice brprice force-pushed the brprice/letrec-typing branch 2 times, most recently from 7875e91 to 4009eeb Compare May 17, 2023 16:30
@brprice brprice force-pushed the brprice/nehole-chk branch 2 times, most recently from afe9a61 to 202dc88 Compare May 17, 2023 16:38
Base automatically changed from brprice/letrec-typing to brprice/let-rename-offer May 22, 2023 16:27
@brprice brprice force-pushed the brprice/let-rename-offer branch from 242534d to e375e09 Compare May 22, 2023 16:34
Base automatically changed from brprice/let-rename-offer to main May 22, 2023 17:01
brprice added 2 commits May 22, 2023 18:09
Previously the typing rule was
   e ∈ S
-----------
{? e ?} ∈ ?

Now the rule is
   ? ∋ e
-----------
{? e ?} ∈ ?

BREAKING CHANGE: since this changes the behaviour of smartholes and the
`SetConFieldType` action, it will change the interpretation of program
logs. Thus it may break replaying logs stored from older versions of
primer (e.g. when using 'undo' on a program loaded from an old
database).

Signed-off-by: Ben Price <ben@hackworthltd.com>
Signed-off-by: Ben Price <ben@hackworthltd.com>
@brprice brprice force-pushed the brprice/nehole-chk branch from 202dc88 to 50dccbf Compare May 22, 2023 17:20
@brprice brprice enabled auto-merge May 22, 2023 17:21
@brprice brprice disabled auto-merge May 22, 2023 17:22
@brprice brprice changed the base branch from main to brprice/api-generate May 22, 2023 17:25
@brprice brprice changed the base branch from brprice/api-generate to main May 22, 2023 17:25
@brprice brprice added this pull request to the merge queue May 22, 2023
Merged via the queue into main with commit 8dd8582 May 22, 2023
@brprice brprice deleted the brprice/nehole-chk branch May 22, 2023 18:03
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