Skip to content

Comments

fix: interpret empty true-expr of conditional as error#3581

Merged
josdejong merged 4 commits intojosdejong:developfrom
gwhitney:fix_3578
Nov 12, 2025
Merged

fix: interpret empty true-expr of conditional as error#3581
josdejong merged 4 commits intojosdejong:developfrom
gwhitney:fix_3578

Conversation

@gwhitney
Copy link
Collaborator

Resolves #3578.

@gwhitney gwhitney changed the title fix: interpret empty true-expr of conditional as undefined fix: interpret empty true-expr of conditional as error Nov 7, 2025
@gwhitney gwhitney marked this pull request as draft November 7, 2025 15:58
@gwhitney gwhitney marked this pull request as ready for review November 7, 2025 22:40
@josdejong
Copy link
Owner

Thanks Glen!

I didn't remember this support for an implict start of a range :3. Maybe we should remove that too according to the same logic of not allowing missing parts 🤔. That would be a breaking change though, and unlike missing a part in a conditional operator, this is actually a useful feature and has a logical default value for the start. I guess it's best to keep supporting it. I'll add a few unit tests for it.

@josdejong josdejong merged commit 7db7912 into josdejong:develop Nov 12, 2025
8 checks passed
@josdejong
Copy link
Owner

Ah, I get it again: when ranges are used inside an index, the start and/or end is optional, so you can enter something like A[2, :] or A[:3]. In that case, a missing range start is interpreted as the start of the matrix, and a missing end range end as the end of the matrix. So it has context.

I think it was never intended to allow ranges used outside of an index to omit the start or end. Right now something like :3 works, but 3: not because there is no end available in the context. In short: it may make sense to make start and end of a range required when not inside of a matrix index. That will be a breaking change though.

@gwhitney gwhitney deleted the fix_3578 branch November 12, 2025 14:42
@gwhitney
Copy link
Collaborator Author

I think it was never intended to allow ranges used outside of an index to omit the start or end. Right now something like :3 works, but 3: not because there is no end available in the context. In short: it may make sense to make start and end of a range required when not inside of a matrix index. That will be a breaking change though.

I would just leave it. :n.forEach(i => ...do stuff with i) is a nice idiom for the ubiquitous loop from 1 to n, and if the Range Matrix PR is eventually worked into shape and merged, it really will be just a loop. Also I could swear that while messing around with that PR i encountered some other example where it would make perfect sense to interpret the end marker outside of indexing a matrix. But I didn't write it down, sadly, because I was focused on other things. So I don't think it is doing any harm and if we leave it, hopefully such examples may come up again.

@gwhitney
Copy link
Collaborator Author

gwhitney commented Nov 12, 2025

In fact, here's one: If you go to the mathjs REPL right now, you have:

A = [9, 8, 7, 6]
    [9, 8, 7, 6]

A[2:4]
    [8, 7, 6]

A[2:]
    [8, 7, 6]

subset(A, index(2:4))
    [8, 7, 6]

subset(A, index(2:))
    Error: Undefined symbol end

I do not see any reason the last one should not work, completing the analogy between the first and third. Should I file an issue for that, or just leave it as a little under-the-covers foible of mathjs?

@josdejong
Copy link
Owner

I do not see any reason the last one should not work, completing the analogy between the first and third. Should I file an issue for that, or just leave it as a little under-the-covers foible of mathjs?

I think the logic here is that a range like 2: is only supported inside the square brackets of a matrix index, like A[2:]. Not in the root of an expression like just 2: , nor as a function argument like index(2:). I think it's fine to leave this is it is right now, unless you strongly like to change this behavior.

@gwhitney
Copy link
Collaborator Author

a range like 2: is only supported inside the square brackets of a matrix index,

I agree. That's the only place that it's supported right now. But I don't see any logic for not supporting it anywhere else it makes perfect sense, such as subset(M, index(2:)). Just that we haven't done the work to support it there. I will see if I can incorporate it into the Range-is-Matrix PR, not sure.

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.

Unexpected behavior with ternary operator syntax a ? : 1 in evaluate()

2 participants