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

Add first draft of const eval skill tree to this repo #43

Merged
merged 19 commits into from Aug 6, 2020
Merged

Conversation

oli-obk
Copy link
Contributor

@oli-obk oli-obk commented May 15, 2020

No description provided.

@RalfJung
Copy link
Member

Can you point to a rendered version of this to make reading the PR easier?

@oli-obk
Copy link
Contributor Author

oli-obk commented Jun 10, 2020

It's a lot of work to set up a webpage. I have no system set up in order to do something like that easily and I couldn't find a throwaway website hoster. I added instructions to the PR for how to render the skill tree.

@jplatte
Copy link

jplatte commented Jun 10, 2020

About to build and host on turbo.fish :)

@nikomatsakis
Copy link

I'll try to get a rendering setup on this repo

@nikomatsakis
Copy link

just saw @jplatte's comment though, please feel free @jplatte to do something easier/faster :)

@jplatte
Copy link

jplatte commented Jun 10, 2020

https://static.turbo.fish/const-eval-skill-tree/skill-tree.html

I have zero experience with GitHub webhooks though, that page is not updated automatically.
Found out I can't create a webhook for sth. that I don't have owner permission on. Will look into just polling the API every few minutes.

EDIT: automated, will check that is catches the next few updates, if it breaks after that let me know.

@oli-obk
Copy link
Contributor Author

oli-obk commented Jun 10, 2020

Thanks <3

@RalfJung
Copy link
Member

Okay so we can look at https://static.turbo.fish/const-eval-skill-tree/skill-tree.html to review the toml file in this branch? That's amazing. :D

@jplatte
Copy link

jplatte commented Jun 10, 2020

Yeah, but it takes up to five minutes for it to update, bc. I can't do webhooks on things I don't have owner access to, so the bot now just checks whether the HEAD of the PR has changed every 5 minutes to determine whether to re-build.

@vertexclique
Copy link
Member

What is this for? Documentation purposes?

@felix91gr
Copy link

@vertexclique I see it as a tool for goal-setting.

When a topic is as complicated as this, instead of holding the entirety of it in your head, a visualization tool like this one is really handy :)

@RalfJung
Copy link
Member

All right, I took a look at this. Looks good overall, though some of it is a bit scattered.

functions that take arguments and convert them to patterns

I had to read this 5 times to understand what you mean.^^ I first thought it was about functions like fn foo((a, b): (i32, i32)) that have pattern arguments... but you mean something like pattern fn.

eliminate rustc_promotable

IMO the longer-term plan here should be to only promote things implicitly which, syntactically, we would also allow in patterns. That's an easy principle, and getting rid of the rustc_promotable attribute is just one part here. With const {} blocks, we don't even need "pattern functions" for this. Honestly with this goal I don't think we want "pattern functions" at all...

[T]::eq

I don't think we can ever run this as-is in CTFE, or at least it needs more than the skill tree currently documents. In Miri, this function was only properly supported after we added intrptrcast! The issue is that it can compare pointers to zero-sized allocations with integers, and there is no way we can answer that question in CTFE.

So I think there are two options:

  • Find a hack that skips the ptr comparison in CTFE.
  • Add a "ptr_maybe_eq" intrinsic, which can always return true when CTFE doesn't know. But this could behave differently between CTFE and runtime!

format!

This also needs a link from "something with (fn) ptr equality"

assert_eq! invocations in consts

Needs a link from format!. Or rather, I'd add an intermediate feature "const panics with string formatting".

Mutex::new

I don't think this has anything to do with const fn; this rather requires something like parking_lot...

@RalfJung
Copy link
Member

I had to read this 5 times to understand what you mean.^^ I first thought it was about functions like fn foo((a, b): (i32, i32)) that have pattern arguments... but you mean something like pattern fn.

Oh, I just realized this is actually more powerful than const {} in patterns:

match foo { some_pattern_fn(x) => x, _ => return }

But that seems fairly orthogonal to me to... all of CTFE, really. It requires a totally different way of "evaluating" functions. So I wouldn't even make it part of the same skill tree.

@oli-obk
Copy link
Contributor Author

oli-obk commented Jun 12, 2020

I don't think we can ever run this as-is in CTFE, or at least it needs more than the skill tree currently documents. In Miri, this function was only properly supported after we added intrptrcast! The issue is that it can compare pointers to zero-sized allocations with integers, and there is no way we can answer that question in CTFE.

Since we made raw ptr comparisons unsafe already in const contexts, we don't need an intrinsic, we can just make that comparison behave this way.

@RalfJung
Copy link
Member

Since we made raw ptr comparisons unsafe already in const contexts, we don't need an intrinsic, we can just make that comparison behave this way.

We could, but that seems even more risky. I'd rather call out more explicitly that this is not the pointer comparison operation one is used to.

@oli-obk
Copy link
Contributor Author

oli-obk commented Jun 29, 2020

@jplatte the skill-tree tool changed how it works. The command to regenerate is now mdbook build and needs a cargo install mdbook && cargo install mdbook-skill-tree before being invoked.

I added some stylesheet magic to cause the arrows to get thicker and colorful when the mouse hovers over them, I think this should address the concern by the lang team about the tree being hard to trace with all of those arrows. This already got a lot better after I removed if/match and loop, but still.

@nikomatsakis
Copy link

@oli-obk if/when this repo lands, we can set it up to auto-publish the mdbook to rust-lang.github.io/const-eval with relative ease. Then you could also add other things to the book if they made sense.

@jplatte
Copy link

jplatte commented Jun 29, 2020

@oli-obk Updating to skill-tree 2.0 + mdbook is more work than I was able to invest today. However, skill-tree-upd did crash and I've restarted and made it a bit more robust now. I hope it continues to be valuable without updating to 2.0, but otherwise I can try to update to 2.0 tomorrow.

@oli-obk
Copy link
Contributor Author

oli-obk commented Jun 30, 2020

@jplatte thanks for all the work you've put into this, it has made discussion in this PR much more effective. I'll figure out how to run this in CI for auto-publishing now.

@RalfJung
Copy link
Member

rust-lang/rust#57563 (comment) doesn't seem to be in the skill tree? Is it worth adding?

@oli-obk
Copy link
Contributor Author

oli-obk commented Jul 16, 2020

added it

@nikomatsakis
Copy link

@oli-obk shall we .. merge this and set it up so that it gets published at rust-lang.github.io/const-eval?

@oli-obk
Copy link
Contributor Author

oli-obk commented Jul 29, 2020

I haven't done the work we talked about at the const eval lang team meeting yet. Most notably not all nodes in the skill tree have a hyperlink to a tracking issue, but maybe it would be better to merge first and then keep adjusting it.

@nikomatsakis
Copy link

We'll have to do a bit of setup to get it published. To be honest I usually just ping @Mark-Simulacrum or @pietroalbini who have access to handy scripts to make that work =)

@Mark-Simulacrum
Copy link
Member

Happy to do the legwork to get this published, just give me a ping on Zulip (#t-infra works) with the repo and domain you want it published at.

name = "transmute"
label = "transmute"
requires = ["unconst_rules"]
items = []
Copy link
Member

Choose a reason for hiding this comment

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

We should probably add const_fn_transmute, or rather make it one node with const_fn_union.

@RalfJung
Copy link
Member

RalfJung commented Aug 5, 2020

I find it hard to review something like this, so I am in favor of merging it (with const_fn_transmute taken care of) and then just iterating on it as we notice oddities & missing bits.

@oli-obk oli-obk merged commit 6ec68c9 into master Aug 6, 2020
@oli-obk oli-obk deleted the skill-tree branch August 6, 2020 09:37
@oli-obk
Copy link
Contributor Author

oli-obk commented Aug 7, 2020

@Mark-Simulacrum I set up github actions to generate the mdbook on the gh-pages branch, but I think we need to configure something in the project in order to actually host them?

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.

None yet

7 participants