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

resolve: Get rid of try_define #31308

Closed
wants to merge 1 commit into from

Conversation

petrochenkov
Copy link
Contributor

This was an unresolved question during review of #30843.
Module reexport sets were not deduplicated, so variant reexports ended up in pairs in these sets, and then in metadata, and then led to "duplicate definition" errors unless duplicate checking were turned off.

cc @jseyfried
r? @nikomatsakis

@jseyfried
Copy link
Contributor

@petrochenkov Nice! Looks good to me.

@jseyfried
Copy link
Contributor

This can cause unexpected behavior (including in some cases duplicate errors) when using a crate with shadowed glob imports (see issue #31337).

We should wait to merge this PR until we resolve #31337.

@nikomatsakis
Copy link
Contributor

/me waits

@bors
Copy link
Contributor

bors commented Feb 11, 2016

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

@nikomatsakis
Copy link
Contributor

Given the decision on #31337 -- should we close this PR?

@petrochenkov
Copy link
Contributor Author

@nikomatsakis
Some parts of #31377 are pure bugfixing, not affecting shadowing rules and not requiring an RFC, for example not emitting shadowed items into metadata. If they are extracted and landed, then this could be landed as well.

@jseyfried
Copy link
Contributor

Items that are partially shadowed (for example, a tuple struct shadowed by a function) currently have to be exported to expose the unshadowed namespace. This PR would still cause duplicate errors in that case.

@petrochenkov
Copy link
Contributor Author

Hm, indeed. Closing this then.

@petrochenkov petrochenkov deleted the resolve branch September 21, 2016 19:48
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.

4 participants