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

Fix a number of bugs related to handling of unnamed packages #705

Merged
merged 4 commits into from
Jun 17, 2016

Conversation

leventov
Copy link
Contributor

No description provided.

…revious version of this method does something nonsense and never works.
@monperrus
Copy link
Collaborator

very interesting PR! can you add failing assertions that demonstrate the bug?

@leventov
Copy link
Contributor Author

I don't understand why tests are so unstable. Why they pass locally and fail in CI. Local results from-ide and from console via mvn test are also different.

@tdurieux
Copy link
Collaborator

tdurieux commented Jun 16, 2016

I maybe because your pull request does not contains all modification present in master.
Can you rebase your PR?
Edit: you PR is up to date, I failed to pull your branch 👎

@GerardPaligot
Copy link
Contributor

I pull our branch and got same errors than Travis, Jenkins or Appveyor.

PR on your PR done here: leventov#5

@leventov
Copy link
Contributor Author

@GerardPaligot please wait for a minute. I will try to put everything together.

leventov and others added 2 commits June 16, 2016 16:13
…t TypeReference-building factory methods return references with unnamed package rather than null package and null declaring type, to preserve invariant that either ref.getPackage() or ref.getDeclaringType() is not null, unless the type reference represent a primitive type class or void.class
@leventov
Copy link
Contributor Author

@monperrus for unnamed package issues it would be difficult for me.

For java.lang importing issues, demonstrating case is easy: declare a class clashing with some class in java.lang, i. e. com.mycompany.String, and use it from another class. It should stay fully-qualified when pretty-printed. Before this PR spoon imports it and uses unqualifed, and output is not compilable because java compiler doesn't know which class to use, com.mycompany.String or java.lang.String.

if (importsContext.isImported(ref)) {
// If java.lang.Something is imported, but we are in the context of a class which is
// also called "Something", we should still use qualified version java.lang.Something
if (ref.getPackage() != null && ref.getPackage().getSimpleName().equals("java.lang")) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

As an afterthought, this shouldn't probably be restricted with java.lang, but for any imported classes.

Copy link
Collaborator

Choose a reason for hiding this comment

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

what do you mean?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Probably this should be checked for all classes, i. e. in terms of code, just remove ref.getPackage().getSimpleName().equals("java.lang") condition

Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok

@monperrus
Copy link
Collaborator

Thanks. @GerardPaligot if you're also OK, do you merge?

@GerardPaligot GerardPaligot merged commit 540b260 into INRIA:master Jun 17, 2016
@GerardPaligot
Copy link
Contributor

Thanks for this contribution @leventov!

@tdurieux tdurieux mentioned this pull request Jun 24, 2016
@leventov leventov deleted the pr_705 branch February 17, 2018 11:07
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