[iwami4438]

Ref: #3311

Before↓

const ErrorSet = error{A} || error{B}
(type = error{A,B})

const ErrorSetWithUnknown = error{A} || undefined.Unknown
(unknown)

const ErrorSetWithUnknown2 = undefined.Unknown || error{A}
(unknown)

const ErrorSetWithUnknown3 = undefined.Unknown || error{A} || error{B}
(unknown)

const AllUnknown = undefined.Unknown || undefined.Unknown
(unknown)

After: ↓

const ErrorSet = error{A} || error{B}
(type = error{A,B})

const ErrorSetWithUnknown = error{A} || undefined.Unknown
(type = error{A, ... })

const ErrorSetWithUnknown2 = undefined.Unknown || error{B}
(type = error{B, ... })

const ErrorSetWithUnknown3 = undefined.Unknown || error{A} || error{B}
(type = error{A,B, ... })

const AllUnknown = undefined.Unknown || undefined.Unknown
(type = error{ ... })

[iwami4438]

There was an error in the test (it has been fixed—the number of ^ symbols was insufficient). Could you please re-run the CI?

[neoto]

I won't comment on the implementation since I'm not familiar with the codebase, but I tested the PR with these test cases and they all crash the server:

const AnyerrorUnknown = anyerror || Unknown;

const UnknownAnyerror = Unknown || anyerror;

const AnyerrorPartiallyUnknown = anyerror || error{A} || Unknown;

At the very least, I think this should now be lifted up from the else branch to the start of the function:

        // Anything merged with anyerror is anyerror.
        if (a_ty == .anyerror_type or b_ty == .anyerror_type) {
            return .anyerror_type;
        }

[iwami4438]

I won't comment on the implementation since I'm not familiar with the codebase, but I tested the PR with these test cases and they all crash the server:

const AnyerrorUnknown = anyerror || Unknown;

const UnknownAnyerror = Unknown || anyerror;

const AnyerrorPartiallyUnknown = anyerror || error{A} || Unknown;

At the very least, I think this should now be lifted up from the else branch to the start of the function:

        // Anything merged with anyerror is anyerror.
        if (a_ty == .anyerror_type or b_ty == .anyerror_type) {
            return .anyerror_type;
        }

Thank you for your feedback.
anyerror was overlooked—my apologies.
I will add a test.

[techatrix]

@iwami4438 Did use LLMs or AI to author anything here? Let it be the source code, commit descriptions or your comments/replies. Also counting AI translation tools.

[iwami4438]

@iwami4438 Did use LLMs or AI to author anything here? Let it be the source code, commit descriptions or your comments/replies. Also counting AI translation tools.

I am using Google Translate for the English text; please excuse any awkward phrasing. I have not used any AI.

[iwami4438]

All tests passed in my environment; could you please run the CI? Thank you.

[techatrix]

Thank you for working on this. The implementation appears to handle all cases correctly although the code could still be improved to avoid unnecessary or redundant work:

• Avoid calling isUnknownDeep. We can use isUnknown instead or since we still assume that the arguments are guaranteed to be either an unknown type or an error set, ty == .unknown_type.
• In the case of error{Foo} || Unknown, the error names are unnecessary placed in a set, sorted and interned again. The interned error names could be reused instead.

Here is a suggestion for how the code could be restructured:

    if (a_ty == .unknown_type and b_ty == .unknown_type) {
        // return error set with `.names = .empty` and set has_unknown
    }

    if (a_ty == .unknown_type) {
        // return error set where the error names are taken from `b_ty` and set has_unknown
    }

    if (b_ty == .unknown_type) {
        // return error set where the error names are taken from `a_ty` and set has_unknown
    }

    // Merge error sets as usual but make sure to resolve `has_unknown` as well.

Also, the resolvePeerErrorSets function in analysis.zig should be adjusted to handle unknown types as well. Also note this review comment in case it's overlooked.

[iwami4438]

@techatrix
Thank you for your suggestion. I understand that isUnknownDeep should not be used. For this particular use case, I have determined that using isUnknown is more appropriate than comparing via .unknown_type, so I will proceed with that approach.
Also, thank you for your suggestion regarding has_unknown.
I will give that a try.

[iwami4438]

I have incorporated the suggested changes. I tested them in my environment, and no errors occurred. Please review.

[iwami4438]

CI failed. Merging #3330 into ZLS, then merging those changes here and pushing them, followed by re-running the CI, should resolve the issue.

[techatrix]

Thank you for following up on the review. I have some comments on further refactors but otherwise looks ready to go.

[techatrix]

It is best to avoid unnecessarily calling indexToKey. There is a previous call to ip.indexToKey(a_ty) and ip.indexToKey(b_ty) that should be reused.

[techatrix]

This check is unnecessary and can be safely removed.