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

Provide better error hint when UndefVarError results from name clashes #53469

Merged

Conversation

ericphanson
Copy link
Contributor

We can detect this since we set the usingfailed bit when the clash occurs (to avoid printing the WARNING multiple times). In this case, typos or missing imports (the current message) isn't quite as clear as it could be, because in fact the name is probably spelled totally right, it's just that there is a missing explicit import or the name should be qualified.

This code will stop working if we change the flags in Core.Binding, but the test I added should catch that. However if REPL is supposed to be independent of Base and not depend on internals there, there could be an issue. In that case we should probably add an API to Base to inspect this usingfailed bit so we can use it in the REPL.

stdlib/REPL/test/repl.jl Outdated Show resolved Hide resolved
Copy link
Member

@vtjnash vtjnash left a comment

Choose a reason for hiding this comment

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

I am okay with this internal package relying on an implementation detail here for this purpose. So looks great to me

@JeffBezanson JeffBezanson added the error messages Better, more actionable error messages label Feb 26, 2024
@JeffBezanson
Copy link
Member

This is great; it's very nice to have a more specific error for this.

stdlib/REPL/test/repl.jl Outdated Show resolved Hide resolved
Co-authored-by: Jameson Nash <vtjnash@gmail.com>
stdlib/REPL/src/REPL.jl Outdated Show resolved Hide resolved
stdlib/REPL/test/repl.jl Outdated Show resolved Hide resolved
Co-authored-by: Alex Arslan <ararslan@comcast.net>
@ericphanson
Copy link
Contributor Author

whitespace check failed with Exited with status -1 (agent lost); locally it passes

@vtjnash vtjnash added the merge me PR is reviewed. Merge when all tests are passing label Feb 28, 2024
@vtjnash
Copy link
Member

vtjnash commented Feb 28, 2024

Seemed like a widespread agent failure, so I restarted those. We can merge when that finishes running (or we could do it sooner, but there is not really any reason to bypass CI).

@IanButterworth IanButterworth merged commit 0f902bf into JuliaLang:master Mar 3, 2024
8 checks passed
@ericphanson ericphanson deleted the eph/better-clash-message branch March 3, 2024 11:26
@IanButterworth IanButterworth removed the merge me PR is reviewed. Merge when all tests are passing label Mar 4, 2024
tecosaur pushed a commit to tecosaur/julia that referenced this pull request Mar 4, 2024
…hes (JuliaLang#53469)

We can detect this since we set the `usingfailed` bit when the clash
occurs (to avoid printing the `WARNING` multiple times). In this case,
typos or missing imports (the current message) isn't quite as clear as
it could be, because in fact the name is probably spelled totally right,
it's just that there is a missing explicit import or the name should be
qualified.

This code will stop working if we change the flags in `Core.Binding`,
but the test I added should catch that. However if REPL is supposed to
be independent of Base and not depend on internals there, there could be
an issue. In that case we should probably add an API to Base to inspect
this `usingfailed` bit so we can use it in the REPL.

---------

Co-authored-by: Jameson Nash <vtjnash@gmail.com>
Co-authored-by: Alex Arslan <ararslan@comcast.net>
mkitti pushed a commit to mkitti/julia that referenced this pull request Apr 13, 2024
…hes (JuliaLang#53469)

We can detect this since we set the `usingfailed` bit when the clash
occurs (to avoid printing the `WARNING` multiple times). In this case,
typos or missing imports (the current message) isn't quite as clear as
it could be, because in fact the name is probably spelled totally right,
it's just that there is a missing explicit import or the name should be
qualified.

This code will stop working if we change the flags in `Core.Binding`,
but the test I added should catch that. However if REPL is supposed to
be independent of Base and not depend on internals there, there could be
an issue. In that case we should probably add an API to Base to inspect
this `usingfailed` bit so we can use it in the REPL.

---------

Co-authored-by: Jameson Nash <vtjnash@gmail.com>
Co-authored-by: Alex Arslan <ararslan@comcast.net>
@IanButterworth IanButterworth added the backport 1.11 Change should be backported to release-1.11 label Oct 10, 2024
@IanButterworth
Copy link
Member

Backporting this so we can backport #56103 too

IanButterworth pushed a commit that referenced this pull request Oct 10, 2024
…hes (#53469)

We can detect this since we set the `usingfailed` bit when the clash
occurs (to avoid printing the `WARNING` multiple times). In this case,
typos or missing imports (the current message) isn't quite as clear as
it could be, because in fact the name is probably spelled totally right,
it's just that there is a missing explicit import or the name should be
qualified.

This code will stop working if we change the flags in `Core.Binding`,
but the test I added should catch that. However if REPL is supposed to
be independent of Base and not depend on internals there, there could be
an issue. In that case we should probably add an API to Base to inspect
this `usingfailed` bit so we can use it in the REPL.

---------

Co-authored-by: Jameson Nash <vtjnash@gmail.com>
Co-authored-by: Alex Arslan <ararslan@comcast.net>
(cherry picked from commit 0f902bf)
KristofferC added a commit that referenced this pull request Oct 15, 2024
Backported PRs:
- [x] #55945 <!-- Fix logic in `?` docstring example -->
- [x] #55932 <!-- REPL: make UndefVarError aware of imported modules -->
- [x] #55968 <!-- [build] avoid libedit linkage and align libccalllazy*
SONAMEs -->
- [x] #55977 <!-- fix comma logic in time_print -->
- [x] #55982 <!-- `@time` actually fix time report commas & add tests
-->
- [x] #55743 <!-- doc: heap snapshot viewing -->
- [x] #55851 <!-- [REPL] Fix #55850 by using `safe_realpath` instead of
`abspath` in `projname` -->
- [x] #55992 <!-- Avoid `stat`-ing stdlib path if it's unreadable -->
- [x] #55589 <!-- prevent loading other extensions when precompiling an
extension -->
- [x] #54009 <!-- allow extensions to trigger from packages in [deps]
-->
- [x] #56019 <!-- Fix no-arg `ScopedValues.@with` within a scope -->
- [x] #56023 <!-- Sockets: Warn when local network access not granted.
-->
- [x] #55569 <!-- Add a docs section about loading/precomp/ttfx time
tuning -->
- [x] #55824 <!-- Replace regex package module checks with actual code
checks -->
- [x] #56041 <!-- Don't show keymap `@error` for hints -->
- [x] #53469
- [x] #56029 <!-- fix `_growbeg!` unncessary resizing -->
- [x] #56103
- [x] #55941 <!-- Fix zero elements for block-matrix kron involving
Diagonal -->
@KristofferC KristofferC removed the backport 1.11 Change should be backported to release-1.11 label Oct 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
error messages Better, more actionable error messages
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants