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

Warn unused type aliases #37631

Closed
wants to merge 3 commits into from
Closed

Conversation

sanxiyn
Copy link
Member

@sanxiyn sanxiyn commented Nov 7, 2016

The interesting part (type aliases used by UFCS) is already tested by issue-23808.rs. But it only tests used type aliases are not warned, so I added a simple test to test unused type aliases are warned.

Fix #37455.

@rust-highfive
Copy link
Collaborator

r? @Aatch

(rust_highfive has picked a reviewer for you, use r? to override)

@petrochenkov
Copy link
Contributor

r? @eddyb
(Overlap with lazy-start)

@eddyb
Copy link
Member

eddyb commented Nov 7, 2016

What's the motivation between the changes to how path resolutions are stored?
I have a somewhat more complex design I've been working on, which satisfies the needs of incremental and on-demand type-checking.
It'd be already up but I'm still trying to untangle the privacy pass from syntactical types.

@sanxiyn
Copy link
Member Author

sanxiyn commented Nov 8, 2016

The motivation is to read partial resolution after it is overwritten by full resolution in def_map, so that UFCS path can keep type alias alive.

It can be achieved by simply keeping a copy of def_map as returned by resolve, but this (def_map only stores full resolution) seemed cleaner to me.

@eddyb
Copy link
Member

eddyb commented Nov 8, 2016

@sanxiyn Ah this is completely unnecessary on my branch, which has a separate HIR node for the original resolution (i.e. the T in T::A). Do you mind waiting for that?

@sanxiyn
Copy link
Member Author

sanxiyn commented Nov 8, 2016

Sure. It would be much better if I can get the original resolution directly from HIR node without maintaining the side table. Thanks.

@eddyb
Copy link
Member

eddyb commented Nov 9, 2016

@sanxiyn #37676 is up, you can probably rebase on top of it if you want (although I should fix Travis first).

@nikomatsakis nikomatsakis assigned eddyb and unassigned Aatch Nov 10, 2016
@brson
Copy link
Contributor

brson commented Nov 11, 2016

Nice fix.

bors added a commit to rust-lang/cargo that referenced this pull request Nov 15, 2016
Remove unused type aliases

Found by rust-lang/rust#37631 and necessary to land because of cargotest.
@bors
Copy link
Contributor

bors commented Nov 17, 2016

☔ The latest upstream changes (presumably #37732) made this pull request unmergeable. Please resolve the merge conflicts.

@bors
Copy link
Contributor

bors commented Nov 28, 2016

☔ The latest upstream changes (presumably #37676) made this pull request unmergeable. Please resolve the merge conflicts.

@eddyb
Copy link
Member

eddyb commented Nov 28, 2016

Sorry for the delay but this should be straight-forward now.

@petrochenkov
Copy link
Contributor

Should this be closed?

@sanxiyn
Copy link
Member Author

sanxiyn commented Dec 17, 2016

I will close this when the reimplementation lands.

bors added a commit that referenced this pull request Dec 18, 2016
 Warn unused type aliases, reimplemented

Reimplementation of #37631. Fix #37455.
@sanxiyn sanxiyn closed this Dec 19, 2016
@sanxiyn sanxiyn deleted the unused-type-alias-2 branch December 19, 2016 02:32
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.

7 participants