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

Add error hint for incorrect stacked indexing #40934

Merged
merged 15 commits into from
Feb 27, 2024

Conversation

BioTurboNick
Copy link
Contributor

@BioTurboNick BioTurboNick commented May 24, 2021

A common entry level mistake is to try to index a matrix with two sets of brackets, e.g. a = [1 2; 3 4]; a[1][2] = 5

This will lead to an error that setindex!() on the element type of a is missing.

This PR adds an error hint for the case where a MethodError is raised when setindex! is called with a Number as the first argument.

I considered going broader than numbers, but it seems more likely that this kind of mistake would happen when working with simple number arrays vs. something more advanced.

Could also consider if it is possible to do the same for when getindex() is called on a Number, which emits a BoundsError.

@simeonschaub simeonschaub added domain:arrays [a, r, r, a, y, s] domain:error messages Better, more actionable error messages labels May 28, 2021
@simeonschaub
Copy link
Member

simeonschaub commented May 28, 2021

This is sort of blocked by #40986 unfortunately. OTOH, maybe we can also merge this like this, since we probably need to move these around in another PR anyways.

@vtjnash
Copy link
Sponsor Member

vtjnash commented Jun 1, 2021

I see a failure:

      From worker 2:	��� Error: Hint-handler nonsetable_number_hint_handler for MethodError in Base caused an error
      From worker 2:	��� @ Base.Experimental experimental.jl:251

@vtjnash
Copy link
Sponsor Member

vtjnash commented Jun 19, 2021

I think we might also want to put this one in the REPL module? It feels undesirable to me to have too much complexity (and diagnostic hints) accumulating in this particular file.

base/errorshow.jl Outdated Show resolved Hide resolved
@vtjnash
Copy link
Sponsor Member

vtjnash commented Jun 19, 2021

Also still showing failures on CI:

      From worker 2:	��� Error: Hint-handler nonsetable_number_hint_handler for MethodError in Base caused an error
      From worker 2:	��� @ Base.Experimental experimental.jl:252

@BioTurboNick
Copy link
Contributor Author

Also still showing failures on CI:

      From worker 2:	��� Error: Hint-handler nonsetable_number_hint_handler for MethodError in Base caused an error
      From worker 2:	��� @ Base.Experimental experimental.jl:252

Where are you seeing that? I see all green checkmarks.

@vtjnash vtjnash added the needs tests Unit tests are required for this change label Feb 12, 2024
base/errorshow.jl Outdated Show resolved Hide resolved
BioTurboNick and others added 2 commits February 12, 2024 16:50
Co-authored-by: Michael Abbott <32575566+mcabbott@users.noreply.github.com>
@vtjnash vtjnash removed the needs tests Unit tests are required for this change label Feb 13, 2024
test/errorshow.jl Outdated Show resolved Hide resolved
@KristofferC
Copy link
Sponsor Member

Could #53241 be fixed together with this by just widening the cases where the error hint matches a bit?

@BioTurboNick
Copy link
Contributor Author

Could #53241 be fixed together with this by just widening the cases where the error hint matches a bit?

Pushed this with my own suggested message - will add the test once it's settled

@vtjnash vtjnash added the status:merge me PR is reviewed. Merge when all tests are passing label Feb 13, 2024
@fingolfin fingolfin closed this Feb 23, 2024
@fingolfin fingolfin reopened this Feb 23, 2024
@fingolfin
Copy link
Contributor

Restarting tests once again (apparently this was already done twice before, so maybe the additional failures are really caused by this PR? We'll see!)

@BioTurboNick
Copy link
Contributor Author

Seems like something else is going on, but I do still need to add tests for the other error hint - was waiting for feedback on if my solution needed any tweaks.

@vtjnash vtjnash removed the status:merge me PR is reviewed. Merge when all tests are passing label Feb 23, 2024
base/errorshow.jl Outdated Show resolved Hide resolved
@vtjnash
Copy link
Sponsor Member

vtjnash commented Feb 23, 2024

I would rather merge this, once you have the other test written, than nit pick the wording until this gets forgotten for 3 more years. It does seem we should consider the tone eventually, but that shouldn't hold up this, since we can always rework it later (e.g. do we use questions or make statements, suggest changes to "you" or to "the code", and should the statements be about what you attempted or about what the code interpreted it as doing or what the alternatives are). Looking at the existing messages, we are inconsistent about the style of message, but maybe that is what is sensible anyways, since the reasoning is different for them.

BioTurboNick and others added 3 commits February 26, 2024 13:26
Co-authored-by: Jameson Nash <vtjnash@gmail.com>
@BioTurboNick
Copy link
Contributor Author

Tests added so should be ready to go.

@vtjnash vtjnash merged commit c19c68e into JuliaLang:master Feb 27, 2024
5 of 7 checks passed
tecosaur pushed a commit to tecosaur/julia that referenced this pull request Mar 4, 2024
A common entry level mistake is to try to index a matrix with two sets
of brackets, e.g. `a = [1 2; 3 4]; a[1][2] = 5`

This will lead to an error that `setindex!()` on the element type of `a`
is missing.

This PR adds an error hint for the case where a MethodError is raised
when `setindex!` is called with a `Number` as the first argument.

I considered going broader than numbers, but it seems more likely that
this kind of mistake would happen when working with simple number arrays
vs. something more advanced.

Could also consider if it is possible to do the same for when
`getindex()` is called on a `Number`, which emits a BoundsError.

Co-authored-by: Michael Abbott <32575566+mcabbott@users.noreply.github.com>
mkitti pushed a commit to mkitti/julia that referenced this pull request Mar 7, 2024
A common entry level mistake is to try to index a matrix with two sets
of brackets, e.g. `a = [1 2; 3 4]; a[1][2] = 5`

This will lead to an error that `setindex!()` on the element type of `a`
is missing.

This PR adds an error hint for the case where a MethodError is raised
when `setindex!` is called with a `Number` as the first argument.

I considered going broader than numbers, but it seems more likely that
this kind of mistake would happen when working with simple number arrays
vs. something more advanced.

Could also consider if it is possible to do the same for when
`getindex()` is called on a `Number`, which emits a BoundsError.

Co-authored-by: Michael Abbott <32575566+mcabbott@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
domain:arrays [a, r, r, a, y, s] domain:error messages Better, more actionable error messages
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants