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.

Sign up for GitHub

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

Jump to bottom

Fix TC allowing bad schemas in bind, with-read, with-capability #1212

Merged
merged 2 commits into from
May 1, 2023
Merged

Fix TC allowing bad schemas in bind, with-read, with-capability #1212

merged 2 commits into from
May 1, 2023

Conversation

sirlensalot
Copy link
Contributor

No description provided.

jwiegley
jwiegley reviewed Apr 30, 2023
View reviewed changes
Copy link
Contributor

@jwiegley jwiegley left a comment

Choose a reason for hiding this comment

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

Looks great to me, I'll kick off a replay now.

@sirlensalot
Copy link
Contributor Author

Looks great to me, I'll kick off a replay now.

This only affects static typechecker so no replay is necessary

@jwiegley
Copy link
Contributor

Thanks, @sirlensalot, that should have been obvious to me.

emilypi
emilypi approved these changes May 1, 2023
View reviewed changes
Copy link
Member

@emilypi emilypi left a comment

Choose a reason for hiding this comment

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

LGTM thanks @sirlensalot 👍

@emilypi emilypi mentioned this pull request May 1, 2023
Merged
1 task
rsoeldner
rsoeldner reviewed May 1, 2023
View reviewed changes
tests/pact/tc.repl Show resolved Hide resolved
tests/pact/tc.repl Show resolved Hide resolved
imalsogreg
imalsogreg approved these changes May 1, 2023
View reviewed changes
Copy link
Contributor

@imalsogreg imalsogreg left a comment

Choose a reason for hiding this comment

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

Great catch! fyi I've verified that my return-types PR now passes tests when rebased on this. (although that's not surprising, because PR-1212 fixes the Pact code that didn't typecheck before).

Replay doesn't need to be involved for PR-1212, but are there any other backward compat issues we need to think about? (e.g. a customer that runs TC and accidentally relied on the previous lax TC behavior?)

@sirlensalot
Copy link
Contributor Author

are there any other backward compat issues we need to think about? (e.g. a customer that runs TC and accidentally relied on the previous lax TC behavior?)

In general breaking repl-only compat is ok, and static TC + FV are even more so as they're supposed to be about correctness first. Finally since this goes with the runtime TC fork that certainly trumps repl compat.

@sirlensalot sirlensalot merged commit eb0e243 into master May 1, 2023
@sirlensalot sirlensalot deleted the bug/fix-binding-tc-allows-bad-object-schema branch May 1, 2023 22:10
@jwiegley jwiegley mentioned this pull request May 4, 2023
Merged
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@emilypi emilypi emilypi approved these changes

@rsoeldner rsoeldner rsoeldner approved these changes

@imalsogreg imalsogreg imalsogreg approved these changes

@jwiegley jwiegley jwiegley approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@sirlensalot @jwiegley @imalsogreg @emilypi @rsoeldner