Skip to content

Import selectors apply API methods allow to unsafely reference symbols via Strings #22673

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

Closed
jchyb opened this issue Feb 27, 2025 · 5 comments
Closed
Assignees
Labels
area:import Issues tied to imports. itype:bug
Milestone

Comments

@jchyb
Copy link
Contributor

jchyb commented Feb 27, 2025

Compiler version

main

Expectation

Import selectors apply API methods allow to reference symbols via their names, which is unsafe and incongruent to the rest of the API (where we reference things via Symbols. We should not pass Strings directly to those apply methods, instead we should rely on Symbols

See: #22457

@jchyb jchyb added itype:bug stat:needs triage Every issue needs to have an "area" and "itype" label labels Feb 27, 2025
@jchyb jchyb self-assigned this Feb 27, 2025
@jchyb jchyb added this to the 3.7.0 milestone Feb 27, 2025
@jchyb jchyb changed the title Import selectors apply API methods allow to unsafely reference symbols via names Import selectors apply API methods allow to unsafely reference symbols via Strings Feb 27, 2025
@bishabosha
Copy link
Member

bishabosha commented Feb 27, 2025

yes this still makes it unclear what is the purpose of this feature - e.g. the import given wildcard - now you would need to pre implicit search those symbols in order to import them?

@Gedochao Gedochao added area:import Issues tied to imports. and removed stat:needs triage Every issue needs to have an "area" and "itype" label labels Mar 4, 2025
@jchyb
Copy link
Contributor Author

jchyb commented Mar 5, 2025

@bishabosha Sorry about the late reply. The use case is being able to dynamically choose what gets imported for the delayed implicit search (like with summonInline), done after inlining. Also it might streamline even simple static additions to implicit scope, like this:

`{
  import something._
  summonInline[T]
}

when creating trees via reflection without having to suddenly switch to quoted code like the above. They will affect only the implicit searches done after inlining, so no additional handling from the compiler should be necessary. SimpleSelectors and RenameSelectors are useless though.

As for this issue itself, I think I panicked before, the current apply methods do match the previously added stable unapply ones. Also, we of course do use Strings directly at times, like in Symbol.unique, or even Symbol.classSymbol, etc.. With how ImportSelectors are implemented in the compiler (and exposed in the reflection api, with the prefix), it really would not make sense to pass symbols there. We might benefit from adding some checks for users whether a passed name exists, as currently incorrect passed String will just make the OmitSelector silently fail (so no crash or anything like that). I’m leaning towards closing this, but I’ ll leave it up for discussion for now.

@som-snytt
Copy link
Contributor

I'm also skeptical about the proposal, but I understand the use case. Maybe the feature is "programmatic unused import warning".

@jchyb
Copy link
Contributor Author

jchyb commented Mar 5, 2025

If that becomes an issue, we can fix it
Edit: by fixing I mean disabling it for the reflection generated Import expressions. I would not want to add more work for anyone of course (only now realized what the concern was).

@jchyb
Copy link
Contributor Author

jchyb commented Mar 12, 2025

Closing the issue. As I mentioned, putting symbols into those apply method does not really make sense, and the current implementation/api seems fine. Those import selectors applies are experimental, so they can still be changed/removed if anything concrete comes up.

@jchyb jchyb closed this as not planned Won't fix, can't repro, duplicate, stale Mar 12, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:import Issues tied to imports. itype:bug
Projects
None yet
Development

No branches or pull requests

4 participants