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

cast now works in VM in more contexts, behaving like in RT; eg: int|pointer <=> ptr T | ref T (eg: enables nimyaml at CT) #15947

Closed
wants to merge 7 commits into from

Conversation

timotheecour
Copy link
Member

@timotheecour timotheecour commented Nov 13, 2020

future work

  • also handle casts involving tyCstring, eg cast[int](someCstring)

example use case

EDIT: and even with cyclic structures involving YAML anchors, see flyx/NimYAML#92 (comment)

(and after #15528 it works with const too)

  • but obviously this is generally useful and makes more code work at CT (without needing to annotate with lots of when nimvm and custom workarounds)

@timotheecour timotheecour changed the title cast now works in VM in more contexts, behaving like in RT; eg: int <=> ptr T | ref T cast now works in VM in more contexts, behaving like in RT; eg: int|pointer <=> ptr T | ref T Nov 13, 2020
@timotheecour timotheecour changed the title cast now works in VM in more contexts, behaving like in RT; eg: int|pointer <=> ptr T | ref T cast now works in VM in more contexts, behaving like in RT; eg: int|pointer <=> ptr T | ref T (eg: enables nimyaml at CT with ref types) Nov 13, 2020
@timotheecour timotheecour changed the title cast now works in VM in more contexts, behaving like in RT; eg: int|pointer <=> ptr T | ref T (eg: enables nimyaml at CT with ref types) cast now works in VM in more contexts, behaving like in RT; eg: int|pointer <=> ptr T | ref T (eg: nimyaml at CT) Nov 13, 2020
@timotheecour timotheecour changed the title cast now works in VM in more contexts, behaving like in RT; eg: int|pointer <=> ptr T | ref T (eg: nimyaml at CT) cast now works in VM in more contexts, behaving like in RT; eg: int|pointer <=> ptr T | ref T (eg: enables nimyaml at CT) Nov 14, 2020
@timotheecour
Copy link
Member Author

ping @Araq , I've rebased to fix bitrot conflicts

changelog.md Show resolved Hide resolved
changelog.md Show resolved Hide resolved
compiler/vm.nim Outdated Show resolved Hide resolved
else: return false
of tyObject:
if isAssign:
let lhs = cast[PNode](address)
Copy link
Member

Choose a reason for hiding this comment

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

Wait what?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's the same as all the other pre-existing cast[ptr T](address)[] in derefPtrToReg, with T = TNode in this case.
The tests exercise all of this, and make CT behavior consistent with RT behavior.

@Araq
Copy link
Member

Araq commented Nov 27, 2020

But I don't mind that nimYaml doesn't work at compile-time all that much. I care more about my compiler not crashing in mysterious ways because "everything works at compile-time". These crashes at runtime are bad enough already but at least then you have valgrind and GDB and so many tools built for it.

How does the average Nim developer debug VM crashes? Previously this was the rule:

  • The Nim compiler crashes. It's our fault. Always.

With your "improved" stuff:

  • The Nim compiler crashes. It might still be your fault because you used cast at compile-time. Or maybe you used some dependency that does that. Come on, this is so much worse, I don't understand why you don't agree.

@timotheecour
Copy link
Member Author

timotheecour commented Nov 28, 2020

PTAL, all code comments addressed.

But I don't mind that nimYaml doesn't work at compile-time all that much

It's not just nimYaml. It's all the when nimvm: workaround that complicate user code/library code that I run into regularly; it's also nim secret and everything that depends on vm. This PR simply makes CT behave like RT in more places, for the same reason js backend should behave like c backend whenever possible: it allows code reuse.

Users shouldn't have to be forced into workarounds involving using gorgeEx + serialization/deserialization just to use some functionality at CT (or patching the compiler to add more one-off vmops logic that could be avoided).

cast is no more dangerous at CT than at RT, but preventing its use just because a user can misuse it just adds un-necessary limitations.

@Araq
Copy link
Member

Araq commented Dec 1, 2020

cast is no more dangerous at CT than at RT, but preventing its use just because a user can misuse it just adds un-necessary limitations.

But it is. At runtime you have tools to mitigate the problems of the lack of memory safety. At compile-time you have no such tools plus you might not even notice that the compiler is corrupted and produces wrong results. If you want cast in the VM, make it memory safe first. How to do that? The details currently elude me, but in principle it should be possible by adding some refcount and scope protection.

@stale
Copy link

stale bot commented Dec 6, 2021

This pull request has been automatically marked as stale because it has not had recent activity. If you think it is still a valid PR, please rebase it on the latest devel; otherwise it will be closed. Thank you for your contributions.

@stale stale bot added the stale Staled PR/issues; remove the label after fixing them label Dec 6, 2021
@stale stale bot closed this Jan 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stale Staled PR/issues; remove the label after fixing them
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants