Simplify lowering #602

detrumi · 2020-09-04T21:36:44Z

Moved lowering functions that just take Env to a single Lower trait with associated type Lowered, to get rid of all the single-use traits. This retains .lower(env) chaining for the most important types.
Changed lowering functions with extra params to freestanding functions. These could in principle be handled by the same trait, by making the argument generic, but that would make it less clear, and having lowering functions that take extra parameters named differenlty is a good thing, so they stand out.
Same for lowering functions that don't take an environment. These often aren't chained anyways, and having names like lower_adt_repr makes the code clearer IMO.
Use macros to reduce boilerplate around LowerParameterMap (also removing some unused functions there) and lower_type_kind (renamed OpaqueTyDefn.identifier to OpaqueTyDefn.name to line things up better)
Re-use lookup_apply_type in ApplyTypeLookup by changing it slightly, to avoid duplicating map lookup code

jackh726

Some nits. But I think I still would prefer the lower_* functions to just implement Lower but not use the environment.

chalk-integration/src/lowering.rs

jackh726 · 2020-09-07T02:09:14Z

chalk-integration/src/lowering.rs

@@ -86,7 +86,8 @@ struct AssociatedTyLookup {
    addl_variable_kinds: Vec<chalk_ir::VariableKind<ChalkIr>>,
 }

-enum ApplyTypeLookup {
+enum ApplyTypeLookup<'k> {
+    Param(&'k chalk_ir::WithKind<ChalkIr, BoundVar>),


Hmm, so. I'm not super sure about this name, because I don't want it to be confused with the rustc Param type.

Hmm, not sure what else to go with, since BoundVar would be confusing as well.

I guess it ends up serving a similar purpose to rustc's Param, so if we can't think of a better name, Param is probably acceptable. Another option: Parameter. Feel free to take it or leave it.

chalk-integration/src/lowering.rs

jackh726 · 2020-09-07T02:26:42Z

chalk-integration/src/lowering.rs

+lower_param_map!(AdtDefn, None);
+lower_param_map!(FnDefn, None);
+lower_param_map!(ClosureDefn, None);
+lower_param_map!(Impl, None);
+lower_param_map!(AssocTyDefn, None);
+lower_param_map!(AssocTyValue, None);
+lower_param_map!(Clause, None);
+lower_param_map!(
+    TraitDefn,
+    Some(chalk_ir::WithKind::new(
+        chalk_ir::VariableKind::Ty(TyKind::General),
+        Atom::from(SELF),
+    ))
+);


chalk-integration/src/lowering.rs

detrumi · 2020-09-07T07:57:43Z

But I think I still would prefer the lower_* functions to just implement Lower but not use the environment.

The problem is that some functions, like lower_fn_abi, are called before we even construct the empty environment.

I've tried having 2 Lower traits, one with environment and one without, but the one without is just less consistent, since it doesn't always return a LowerResult<_>, and I think mixing .lower() and .lower(env)? would be confusing.

jackh726 · 2020-09-07T08:54:56Z

I don't think having two separate traits is all that bad. We're looking for the solution that best conveys what's going on in the lowering as a whole. Having one or two traits implemented for most things makes more sense that one trait for some and a bunch of functions for everything else. (I'm curious if we could refactor away the arguments in all of the functions, but I'm purposely that out from this PR, because I would like to land us somewhere that is definitely better than get us stuck on what ifs).

I think we can always just return a LowerResult, even if it's always Ok wrapped. For this bit of Chalk, readability is more important that really anything else. If lower() and lower(env) is too confusing (I don't think so), then we could name them differently.

Another thing worth thinking about is which of the functions must be called before the environment is constructed. Then, these could be special and everything else takes an env. (This somewhat depends on how much there are and such)

detrumi · 2020-09-07T10:41:28Z

I don't think having two separate traits is all that bad

👍

I think we can always just return a LowerResult, even if it's always Ok wrapped

This is awkward when lowering vecs, so let's go with this instead, even if it's a bit inconsistent:

trait Lower {
    type Lowered;

    fn lower(&self) -> Self::Lowered;
}

Another thing worth thinking about is which of the functions must be called before the environment is constructed. Then, these could be special and everything else takes an env. (This somewhat depends on how much there are and such)

Turns out that the fn_def_abis part wasn't even needed, so this isn't a problem after all. Entry points like lower_goal are a bit trickier (because it ends with goal.lower(&env)), but maybe that's something to fix in another PR.

jackh726

Just a couple more minor comments. But this looks great. Feel free to merge with/without comments addressed.

jackh726 · 2020-09-07T16:24:41Z

chalk-integration/src/lowering.rs

@@ -86,7 +86,8 @@ struct AssociatedTyLookup {
    addl_variable_kinds: Vec<chalk_ir::VariableKind<ChalkIr>>,
 }

-enum ApplyTypeLookup {
+enum ApplyTypeLookup<'k> {
+    Param(&'k chalk_ir::WithKind<ChalkIr, BoundVar>),


I guess it ends up serving a similar purpose to rustc's Param, so if we can't think of a better name, Param is probably acceptable. Another option: Parameter. Feel free to take it or leave it.

jackh726 · 2020-09-07T16:26:52Z

chalk-integration/src/lowering.rs

-            return Err(RustIrError::NotStruct(name.clone()));
+            .cast(interner)),
+            Err(_) => {
+                if let Some(id) = self.foreign_ty_ids.get(&name.str) {


So, how did this end up on this path (I would expect it to be returned in lookup_apply_type)? Is it just because these can't have generics?

I guess so, since there's no foreign_ty_kinds. Possibly lookup_apply_type and lookup_generic_arg should just use their own lookups, but it's kind of awkward to have all these separate maps.

detrumi · 2020-09-07T16:41:07Z

@bors r+

jackh726 · 2020-09-07T18:21:37Z

@bors r=jackh726

bors · 2020-09-07T18:21:38Z

📌 Commit b1c6c4e has been approved by jackh726

detrumi · 2020-09-07T19:23:40Z

Wait, that didn't merge it? Huh, I don't really get how bors works

jackh726 · 2020-09-08T00:55:38Z

Wait, that didn't merge it? Huh, I don't really get how bors works

I think there's some bors weirdness happening right now.

detrumi force-pushed the simplify-lowering branch from 594b215 to aee7742 Compare September 5, 2020 09:32

detrumi added 4 commits September 5, 2020 11:33

Simplify lowering where clauses

7f4e55e

Reuse a single Lower trait

4ac21d1

Lower and LowerInEnv traits

d657a5d

Simplify kind lowering

cb5168a

detrumi force-pushed the simplify-lowering branch from aee7742 to 9f1fd2f Compare September 5, 2020 09:33

Remove unused code

7c21b0b

detrumi force-pushed the simplify-lowering branch 2 times, most recently from 7a30599 to 7c21b0b Compare September 5, 2020 10:37

detrumi added 8 commits September 5, 2020 12:44

Interner consistency and use Lower trait more

132b668

Remove single-use traits from lowering

e43b518

Change lowering without env to free functions

7963db6

Add default for LowerParameterMap.synthetic_parameters

37d2a52

Use macro to deduplicate lower_type_kind code

423dc6d

Rename LowerInEnv back to Lower

19f2469

Re-use lookup_apply_type to simplify lookup_generic_arg

76ae5a8

Use a macro for LowerParameterMap impls

944ba7a

detrumi changed the title ~~[WIP] Simplify lowering~~ Simplify lowering Sep 5, 2020

detrumi marked this pull request as ready for review September 5, 2020 15:41

Fix incorrect type kinds

623c268

jackh726 requested changes Sep 7, 2020

View reviewed changes

Changes from review

b68aa38

Remove unused fn_def_abis

8334b1a

Rewrite lower_* functions to Lower impls

b1c6c4e

jackh726 approved these changes Sep 7, 2020

View reviewed changes

Rename Param to Parameter

db471f1

detrumi merged commit 99ea62d into rust-lang:master Sep 7, 2020

detrumi deleted the simplify-lowering branch September 7, 2020 19:23

bors mentioned this pull request Sep 7, 2020

Extend well-known traits by Unpin #603

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Simplify lowering #602

Simplify lowering #602

detrumi commented Sep 4, 2020 •

edited

Loading

jackh726 left a comment

jackh726 Sep 7, 2020

detrumi Sep 7, 2020

jackh726 Sep 7, 2020

jackh726 Sep 7, 2020

detrumi commented Sep 7, 2020 •

edited

Loading

jackh726 commented Sep 7, 2020

detrumi commented Sep 7, 2020

jackh726 left a comment

jackh726 Sep 7, 2020

jackh726 Sep 7, 2020

detrumi Sep 7, 2020

detrumi commented Sep 7, 2020

jackh726 commented Sep 7, 2020

bors commented Sep 7, 2020

detrumi commented Sep 7, 2020

jackh726 commented Sep 8, 2020

Simplify lowering #602

Simplify lowering #602

Conversation

detrumi commented Sep 4, 2020 • edited Loading

jackh726 left a comment

Choose a reason for hiding this comment

jackh726 Sep 7, 2020

Choose a reason for hiding this comment

detrumi Sep 7, 2020

Choose a reason for hiding this comment

jackh726 Sep 7, 2020

Choose a reason for hiding this comment

jackh726 Sep 7, 2020

Choose a reason for hiding this comment

detrumi commented Sep 7, 2020 • edited Loading

jackh726 commented Sep 7, 2020

detrumi commented Sep 7, 2020

jackh726 left a comment

Choose a reason for hiding this comment

jackh726 Sep 7, 2020

Choose a reason for hiding this comment

jackh726 Sep 7, 2020

Choose a reason for hiding this comment

detrumi Sep 7, 2020

Choose a reason for hiding this comment

detrumi commented Sep 7, 2020

jackh726 commented Sep 7, 2020

bors commented Sep 7, 2020

detrumi commented Sep 7, 2020

jackh726 commented Sep 8, 2020

detrumi commented Sep 4, 2020 •

edited

Loading

detrumi commented Sep 7, 2020 •

edited

Loading