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.

Sign up for GitHub

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

Jump to bottom

Finer grained accessibility check for auto-imports #6765

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Finer grained accessibility check for auto-imports #6765

wants to merge 5 commits into from
+204 −27

Conversation

masonedmison
Copy link
Contributor

@masonedmison masonedmison commented Sep 11, 2024 •
edited
Loading

closes #6732

TODOs

  • Share "is accessible" logic between filtering for searchOutline and SymbolSearch#search for Scala 2
  • Implement refined filtering for Scala 3

@masonedmison masonedmison mentioned this pull request Sep 11, 2024
Open
@masonedmison masonedmison changed the title Finer grained acessibility check when considering auto-imports Finer grained acessibility check for auto-imports Sep 11, 2024
tgodzik
tgodzik requested changes Sep 12, 2024
View reviewed changes
Copy link
Contributor

@tgodzik tgodzik left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution! Great work!

mtags/src/main/scala-2/scala/meta/internal/pc/AutoImportsProvider.scala Outdated Show resolved Hide resolved
@masonedmison masonedmison changed the title Finer grained acessibility check for auto-imports Finer grained accessibility check for auto-imports Sep 12, 2024
@masonedmison masonedmison marked this pull request as ready for review September 12, 2024 14:59
@masonedmison
Copy link
Contributor Author

@tgodzik Implemented your suggestions above and also made similar changes for Scala 3. This should be ready for review now :)

@tgodzik
Copy link
Contributor

tgodzik commented Sep 13, 2024

Scala CLI and Mill tests are flaky, but it seems cross are indeed failing on:

[error] 	tests.pc.CompletionWorkspaceSuite
[error] 	tests.pc.CompletionArgSuite
[error] 	tests.pc.CompletionSuite
[error] 	tests.pc.CompletionSnippetSuite
[error] 	tests.pc.ShadowingCompletionSuite

Some of that might actually be fixes. If you have any issues with those tests I can take a look.

@masonedmison
Copy link
Contributor Author

Scala CLI and Mill tests are flaky, but it seems cross are indeed failing on:

[error] 	tests.pc.CompletionWorkspaceSuite
[error] 	tests.pc.CompletionArgSuite
[error] 	tests.pc.CompletionSuite
[error] 	tests.pc.CompletionSnippetSuite
[error] 	tests.pc.ShadowingCompletionSuite

Some of that might actually be fixes. If you have any issues with those tests I can take a look.

@tgodzik I'll start having a look at the cross tests.

I'm actually get consistent failures running the "mill" tests locally, too; all of the failures appear to be because of java.util.NoSuchElementException: key not found: mill-bsp. Is that expected?

@tgodzik
Copy link
Contributor

tgodzik commented Sep 13, 2024

I'm actually get consistent failures running the "mill" tests locally, too; all of the failures appear to be because of java.util.NoSuchElementException: key not found: mill-bsp. Is that expected?

That's not expected, but maybe your local mill version influences that? Some leftover one?

@masonedmison
Copy link
Contributor Author

Alright, just pushed up two commits that should fix the tests mentioned above.

[error] 	tests.pc.CompletionWorkspaceSuite
[error] 	tests.pc.CompletionArgSuite
[error] 	tests.pc.CompletionSuite
[error] 	tests.pc.CompletionSnippetSuite
[error] 	tests.pc.ShadowingCompletionSuite

Still can't get the Mill and ScalaCLI tests to pass locally 🤔.

🤞

@tgodzik
Copy link
Contributor

tgodzik commented Sep 13, 2024

Alright, just pushed up two commits that should fix the tests mentioned above.

[error] 	tests.pc.CompletionWorkspaceSuite
[error] 	tests.pc.CompletionArgSuite
[error] 	tests.pc.CompletionSuite
[error] 	tests.pc.CompletionSnippetSuite
[error] 	tests.pc.ShadowingCompletionSuite

Still can't get the Mill and ScalaCLI tests to pass locally 🤔.

🤞

Thanks! I need to anyway double check, since they are quite flaky.

@masonedmison
Copy link
Contributor Author

arg, though I fixed some tests, I've broken some of the others. There are still legitimate failures in cross. I'll look at this in the next day or so.

@masonedmison
Copy link
Contributor Author

@tgodzik The cross tests are now passing. In short, it seems that since we are using isAccessibleFrom as part of CompilerSearchVisitor, some of the "outputs" for completion and auto-imports have changed slightly. These test changes are isolated in separate commits. If anything is suspicious, let me know, and I'll be happy to have a deeper look.

It looks like the Mill Integration tests are still failing, are the failures here expected? Let me know if now and I can have a look. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tgodzik tgodzik tgodzik requested changes

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
backport-scala3-pc
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Quick fix "Import" does not work for modifier private with access qualifier
2 participants
@masonedmison @tgodzik