Remove unnecessary `self_ty` parameter to `get_blanket_impls` #81349

jyn514 · 2021-01-24T18:49:54Z

It can be calculated when necessary at the callsite, there's no need to
pass it separately.

This also renames param_env_def_id to item_def_id.

cc @eddyb

rust-highfive · 2021-01-24T18:49:57Z

r? @ollie27

(rust-highfive has picked a reviewer for you, use r? to override)

jyn514 · 2021-01-24T18:51:28Z

@eddyb you have this fixme in rustdoc:

    // FIXME(eddyb) figure out a better way to pass information about
    // parametrization of `ty` than `param_env_def_id`.

Why is param_env_def_id wrong? Why would there need to be a better way?

src/librustdoc/clean/auto_trait.rs

src/librustdoc/clean/blanket_impl.rs

src/librustdoc/clean/utils.rs

jyn514 · 2021-02-11T17:53:33Z

@camelid this is finally ready for review again :)

camelid · 2021-02-12T02:12:59Z

I may not be able to review this PR since I'm not very familiar with the code, but I'll assign myself so I don't forget about it altogether :)

@ollie27 Feel free to review/approve this if you want to — I just assigned myself because otherwise I'll never remember to look at it.

src/librustdoc/clean/blanket_impl.rs

src/librustdoc/clean/utils.rs

camelid · 2021-02-25T04:37:45Z

src/librustdoc/clean/auto_trait.rs

-        param_env_def_id: DefId,
+        item_def_id: DefId,


Why was this change made? Did the meaning change, or is item_def_id closer to the meaning than param_env_def_id? Do ParamEnvs even have DefIds?

Sorry for all the questions; I know almost nothing about ParamEnv :)

item_def_id is closer to the meaning. All items have a ParamEnv, but the same ParamEnv may be used for things other than items (e.g. each expression in the body of a function). Before, there was a semantic mismatch in the representation: it passed in a Ty, but not all types are items (for example &T). So to get the param env of the type it had to pass in the DefId too. Now it passes in the DefId only, which allows getting both the param env and the type without any additional info.

How will get_blanket_impls behave now for blanket impls like impl Foo for &Struct where the type is &Struct but the type_of the item_def_id is Struct? (Unless I'm misunderstanding what the item_def_id is referring to.)

The same as it did before, all this does is rename the parameter and move the type_of into the function. I don't know exactly how it worked before.

camelid · 2021-02-26T02:43:03Z

Could you take a look at #81349 (comment)?

@rustbot label: -S-waiting-on-review +S-waiting-on-author

camelid · 2021-02-26T02:44:08Z

@eddyb you have this fixme in rustdoc:
    // FIXME(eddyb) figure out a better way to pass information about
    // parametrization of `ty` than `param_env_def_id`.
Why is param_env_def_id wrong? Why would there need to be a better way?

Do you want to follow up with eddyb or someone else about this?

jyn514 · 2021-02-26T07:25:08Z

Do you want to follow up with eddyb or someone else about this?

I would rather not block on it, eddyb is very behind on github notifications.

camelid · 2021-02-27T22:27:09Z

I'm not sure if I'd feel comfortable approving this on my own given my lack of familiarity with ParamEnv, as well as eddyb's FIXME. If you can get someone with knowledge of ParamEnv to look it over I would feel more comfortable approving.

jyn514 · 2021-02-27T22:30:44Z

r? @lcnr

lcnr

some nits

r=me after that

src/librustdoc/clean/auto_trait.rs

It can be calculated when necessary at the callsite, there's no need to pass it separately. This also renames `param_env_def_id` to `item_def_id`.

jyn514 · 2021-02-28T16:12:08Z

@bors r=lcnr

bors · 2021-02-28T16:12:09Z

📌 Commit c47c2c6 has been approved by lcnr

bors · 2021-02-28T19:19:13Z

⌛ Testing commit c47c2c6 with merge e37a13c...

bors · 2021-02-28T22:02:56Z

☀️ Test successful - checks-actions
Approved by: lcnr
Pushing e37a13c to master...

jyn514 added C-cleanup Category: PRs that clean code up or issues documenting cleanup. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. labels Jan 24, 2021

rust-highfive assigned ollie27 Jan 24, 2021

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 24, 2021

jyn514 force-pushed the blanket-impls-cleanup branch from d09b3c6 to d5c7273 Compare January 24, 2021 18:50

camelid reviewed Jan 25, 2021

View reviewed changes

src/librustdoc/clean/auto_trait.rs Outdated Show resolved Hide resolved

src/librustdoc/clean/blanket_impl.rs Outdated Show resolved Hide resolved

src/librustdoc/clean/utils.rs Outdated Show resolved Hide resolved

This comment has been minimized.

Sign in to view

camelid added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Jan 28, 2021

camelid self-assigned this Jan 29, 2021

camelid removed their assignment Feb 7, 2021

jyn514 force-pushed the blanket-impls-cleanup branch 2 times, most recently from 918315a to dd6130a Compare February 11, 2021 17:53

jyn514 removed the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Feb 11, 2021

camelid self-assigned this Feb 12, 2021

This comment has been minimized.

Sign in to view

camelid added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 19, 2021

jyn514 force-pushed the blanket-impls-cleanup branch from dd6130a to 3dfdce3 Compare February 23, 2021 03:58

jyn514 added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 23, 2021

jyn514 force-pushed the blanket-impls-cleanup branch from 3dfdce3 to 85917f4 Compare February 23, 2021 03:59

This comment has been minimized.

Sign in to view

jyn514 force-pushed the blanket-impls-cleanup branch from 85917f4 to 880e8f4 Compare February 23, 2021 20:37

camelid reviewed Feb 25, 2021

View reviewed changes

camelid added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 25, 2021

rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 26, 2021

jyn514 added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 26, 2021

rust-highfive assigned lcnr and unassigned ollie27 and camelid Feb 27, 2021

lcnr approved these changes Feb 27, 2021

View reviewed changes

src/librustdoc/clean/auto_trait.rs Outdated Show resolved Hide resolved

src/librustdoc/clean/auto_trait.rs Outdated Show resolved Hide resolved

camelid added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 27, 2021

Remove unnecessary self_ty parameter to get_blanket_impls

c47c2c6

It can be calculated when necessary at the callsite, there's no need to pass it separately. This also renames `param_env_def_id` to `item_def_id`.

jyn514 force-pushed the blanket-impls-cleanup branch from 0914acb to c47c2c6 Compare February 28, 2021 16:11

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 28, 2021

bors added the merged-by-bors This PR was explicitly merged by bors. label Feb 28, 2021

bors merged commit e37a13c into rust-lang:master Feb 28, 2021

rustbot added this to the 1.52.0 milestone Feb 28, 2021

This was referenced Feb 28, 2021

Remove the dummy cache in DocContext; delete RenderInfo #82018

Merged

Remove RefCell around module_trait_cache #82402

Merged

rustdoc: Get TyCtxt from self instead of passing as argument in AutoTraitFinder #82597

Merged

jyn514 deleted the blanket-impls-cleanup branch February 28, 2021 23:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove unnecessary `self_ty` parameter to `get_blanket_impls` #81349

Remove unnecessary `self_ty` parameter to `get_blanket_impls` #81349

jyn514 commented Jan 24, 2021

rust-highfive commented Jan 24, 2021

jyn514 commented Jan 24, 2021

This comment has been minimized.

jyn514 commented Feb 11, 2021

camelid commented Feb 12, 2021 •

edited

Loading

This comment has been minimized.

This comment has been minimized.

camelid Feb 25, 2021

jyn514 Feb 25, 2021

camelid Feb 25, 2021

jyn514 Feb 26, 2021

camelid commented Feb 26, 2021

camelid commented Feb 26, 2021

jyn514 commented Feb 26, 2021

camelid commented Feb 27, 2021

jyn514 commented Feb 27, 2021

lcnr left a comment

jyn514 commented Feb 28, 2021

bors commented Feb 28, 2021

bors commented Feb 28, 2021

bors commented Feb 28, 2021

Remove unnecessary self_ty parameter to get_blanket_impls #81349

Remove unnecessary self_ty parameter to get_blanket_impls #81349

Conversation

jyn514 commented Jan 24, 2021

rust-highfive commented Jan 24, 2021

jyn514 commented Jan 24, 2021

This comment has been minimized.

jyn514 commented Feb 11, 2021

camelid commented Feb 12, 2021 • edited Loading

This comment has been minimized.

This comment has been minimized.

camelid Feb 25, 2021

Choose a reason for hiding this comment

jyn514 Feb 25, 2021

Choose a reason for hiding this comment

camelid Feb 25, 2021

Choose a reason for hiding this comment

jyn514 Feb 26, 2021

Choose a reason for hiding this comment

camelid commented Feb 26, 2021

camelid commented Feb 26, 2021

jyn514 commented Feb 26, 2021

camelid commented Feb 27, 2021

jyn514 commented Feb 27, 2021

lcnr left a comment

Choose a reason for hiding this comment

jyn514 commented Feb 28, 2021

bors commented Feb 28, 2021

bors commented Feb 28, 2021

bors commented Feb 28, 2021

Remove unnecessary `self_ty` parameter to `get_blanket_impls` #81349

Remove unnecessary `self_ty` parameter to `get_blanket_impls` #81349

camelid commented Feb 12, 2021 •

edited

Loading