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

Refine the return type of hash-copy and for/hashs. #1081

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

Conversation

NoahStoryM
Copy link
Contributor

hash-set, hash-set*, hash-update, hash-remove, and hash-clear should receive an immutable hash table and return an immutable hash table;
hash-set!, hash-set*!, hash-update! , hash-remove!, and hash-clear! should receive a mutable hash table and modify its state;
hash-copy should return a mutable hash table;

for/hash[eq/eqv]: should return an immutable hash table. In the older definition, if a.ty is false, for/hash can't make sure the returned hash table is immutable.

@samth
Copy link
Member

samth commented Apr 26, 2021

This seems like it breaks existing code, as evidenced by the PRs for other repositories. That doesn't seem like something we want to do, at least not when the breakage seems substantial as it is here.

We might want to try to warn people when they do this, which perhaps might eventually let us make these changes. Unfortunately there isn't currently a good way to surface warnings to users in Racket, so there are some design considerations to be addressed first.

@NoahStoryM
Copy link
Contributor Author

I can see this difficulty. In fact, I thought about creating the PR when I discovered that there was a hidden bug in my code which should be thrown by type system. Unfortunately it seems to break too many repositories so maybe it's not a good idea to merge it now.

@NoahStoryM
Copy link
Contributor Author

NoahStoryM commented Oct 5, 2021

I think it's safe to refine operators' return types. And the default return type of for[*]/hash[eq/eqv]: should be #'(Immutable-HashTable Any Any). In this way, we can combine the definitions of define-for/hash:-variant and define-for*/hash:-variant.

@NoahStoryM NoahStoryM changed the title Modified some hash operators' types. Refine the return type of hash-copy and for/hashs. Jun 25, 2022
@NoahStoryM
Copy link
Contributor Author

Hi @samth, currently this PR only optimizes the return type, I think this change is safe. Will TR consider merging it?

@samth
Copy link
Member

samth commented Jun 26, 2022

This has some conflicts, and also the changes to the type annotations in the tests worry me.

@NoahStoryM
Copy link
Contributor Author

the changes to the type annotations in the tests worry me.

I thought (Immutable-HashTable A B) is the subtype of (HashTable A B) and these changes should be safe, so I didn't undo the changes to the tests.

This has some conflicts

I thought (Immutable-HashTable Any Any) is the supertype of all immutable hash tables, so it could be used as the default return type when there is no type annotation. I'm not sure where the conflict is, is there any example?

@samth
Copy link
Member

samth commented Jun 27, 2022

The conflict is a merge conflict, with changes to the files that have happened in the mean time.

I would like to not change the tests, except perhaps for more precise types for the results.

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.

2 participants