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

hashes for refs should be an opt-in feature #18098

Merged
merged 1 commit into from
May 30, 2021
Merged

hashes for refs should be an opt-in feature #18098

merged 1 commit into from
May 30, 2021

Conversation

narimiran
Copy link
Member

@narimiran narimiran commented May 25, 2021

refs #17858:

This does not resolve the issue really, of breaking working code in minor releases - the new hash function must be opt-in in order for that to happen

This is a temporary solution, @Araq has something better in the works.

@timotheecour
Copy link
Member

timotheecour commented May 25, 2021

This is not a good change. The fact that hash(ref) was missing was arguably a weird omission given that hash(ptr) and hash(pointer) work (and was inconsistent with the fact that == works with ptr|pointer|ref).

We should fix stdlib and fix bugs/mis-designs instead of piling up historical artifacts just because (i still haven't seen a bug report showing some actual code that broke in their project as a result of this).

Users that need custom hash for ref type (even generic ones) can already do this and #17731 contains runnableExamples for that, this still works. And it's easy enough to add -d:nimLegacyNoHashRef in you user/project/file config, or even in code via {.define(nimLegacyNoHashRef).}, as a temporary workaround.

Turning the correct behavior as opt-in instead of opt-out is what will create dialects, not the other way around.

@Araq Araq merged commit 50e98e6 into devel May 30, 2021
@Araq Araq deleted the miran-no-ref-hash branch May 30, 2021 21:55
@Araq
Copy link
Member

Araq commented May 30, 2021

Holidays are over, I'll come up with a good solution but in the meantime, this solution is less risky.

@arnetheduck
Copy link
Contributor

arnetheduck commented Jun 2, 2021

The way to introduce breaking changes such as these without flags is to put them in a separate module - this doesn't create dialects and works for composing libraries that rely on different "versions". A flag is almost never the right solution, in any direction, but an "enable" flag is less harmful than a "disable" flag.

This is a reality of the Nim language that maintainers of the std lib in particular should be aware of, that open generic symbols cannot be introduced compatibly, specially not for central parts of the std lib like hash, because inevitably, they will be sandwitched into the wrong places with difficult-to-debug runtime issues. It's good to see that compatibility is taken seriously, it's of paramount importance for any longer-term usage of the language.

(of course, introducing a new module is a good workaround while other solutions are being explored)

@timotheecour timotheecour mentioned this pull request Jul 10, 2021
PMunch pushed a commit to PMunch/Nim that referenced this pull request Mar 28, 2022
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