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

update table of known good syms for contracts #4779

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

gus-massa
Copy link
Contributor

Checklist
  • Feature
  • tests included

Description of change

I'm updating the table of known good symbols/predicates for contracts. Some are new primitives added during this years, other are comparisons that now can have a single argument, and there are a few special cases that don't have a ?, for example not and =.

I'm not sure about the title of the commit:

  • update table of known good syms for contracts
  • update table of known good symbols for contracts
  • update table of known good predicates for contracts
  • other?

Does this need test? Where should they be?

I'll squash both commits before merging. The difference is that the first is an automatic update using the code in helpers.rkt and the second is a manual list.

@sorawee
Copy link
Collaborator

sorawee commented Oct 3, 2023

I wonder why we hard code the list, instead of generating it at compile-time. Is the generation too expensive?

@gus-massa
Copy link
Contributor Author

No idea. We should ask @rfindler

@rfindler
Copy link
Member

rfindler commented Oct 4, 2023

It does sound like something I'd have tried, but I don't know if I did or why I gave up!

As for the entries, I'm not sure about functions like = that raise errors (eg (= "not a number"). (oh, actually-- maybe that's why it isn't automatically generated!). Judging from a comment in the source, it may be that opt/c will generate errors in the wrong order possibly?

@sorawee
Copy link
Collaborator

sorawee commented Oct 4, 2023

Some of the existing entries raise errors though, like (char-alphabetic? 1).

@gus-massa
Copy link
Contributor Author

Another one that already was in the list is (port-closed? "hello").

@rfindler
Copy link
Member

rfindler commented Oct 4, 2023

I took a closer look and I don't see how raising these errors could break what opt/c is doing so I think we're good to go here.

And maybe the reason I didn't do this programmatically was because I was worried about errors but since they were already there, perhaps it is possible to autogenerate the list by just looking at the arity of exports from racket/base (or possibly other things that the contract system already depends on).

@rfindler
Copy link
Member

rfindler commented Oct 4, 2023

As for test cases, much of the contract system's test suite is already using stuff that'll end up in this list. So we might want to add a few test cases of procedures that aren't in this list in various places (locally defining some function with a note that the test case is there precisely for that reason).

@rfindler
Copy link
Member

rfindler commented Oct 4, 2023

Looking over the code, it seems like there could be better use of the known-good contracts, too. Right now, it looks like they're used only in opt/c (which doesn't see much use) and in a check done in region.rkt. I guess the check done in region.rkt should be being done in all uses of coerce-contract. I also wonder if there might be a way to improve -> so that, when we have a known-good contract, we can generate a call to the predicate directly.

Right now there are some obstacles there, as a new set of functions that are like build-very-simple-> would be required. I'm also not sure how much performance there is to be gained along this route.

There's also the "plus-one-acceptor" route (these functions are generated when contract-out sees a suitably simple use of -> and we avoid the chaperone wrapper when such functions are called directly in the importing modules) but looking at the expansion it sure does seem like there would have to be a lot of changes to get the information from this list into a useful place for the macro to generate a direct call to the functions. Alas.

@shhyou shhyou added the pr:needs-work A PR that's wanted but needs work to be mergeable. label Oct 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr:needs-work A PR that's wanted but needs work to be mergeable.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants