net/url: Panic in (nil).ResolveReference(…)

[wking]

Please answer these questions before submitting your issue. Thanks!

What version of Go are you using (go version)?

$ go version
go version go1.7.4 linux/amd64

Does this issue reproduce with the latest release?

Yes.

What operating system and processor architecture are you using (go env)?

This bug is OS-agnostic.

What did you do?

https://play.golang.org/p/grCVrmNOxz

What did you expect to see?

ResolveReference doesn't return an error, so the best we can do is probably return nil if the argument is nil or the argument is relative-enough that we'd need to pull anything off the base object.

What did you see instead?

A panic, but only in cases where the argument is nil, or when the argument is relative-enough that we'd need to pull something of a nil base object.

[ianlancetaylor]

I don't see anything wrong with a panic here. Don't call ResolveReference on a nil URL. That rule is no different than "don't dereference a nil pointer."

[wking]

On Thu, Oct 12, 2017 at 04:37:11PM +0000, Ian Lance Taylor wrote: I don't see anything wrong with a panic here. Don't call `ResolveReference` on a `nil` `URL`. That rule is no different than "don't dereference a nil pointer."

I don't think it's that clearcut, because as the absoluteReference resolution in my example [1] shows, a nil base *works* when the reference is absolute (and no information from the base is needed). Labeling the cases: a. Real base, sufficiently absolute reference: resolved URI matches the initial reference. b. nil base, sufficiently absolute reference: resolved URI matches the initial reference. c. Real base, relative reference: resolved URI combines components from both input URIs. d. nil base, relative reference: currently panics, but I'd like to see nil returned instead. e. Real base, nil relative reference: currently panics, and I'm fine with either panics or a returned nil. f. nil base, nil relative reference: currently panics, and I'm fine with either panics or a returned nil. (d) is this issue, and (b) is a nil-base that doesn't panic in master. What I'm looking for is a way to cleanly error (not panicking) in case (d). There's a semi-real-world example in wking/casengine@4d8f93e8. I guess I could recover the panic, but I don't want to accidentally recover from a *different* panic. [1]: https://play.golang.org/p/grCVrmNOxz

[ianlancetaylor]

I don't think we should return nil. For users who are unaware of the change, that simply pushes the breakage elsewhere. Breaking earlier is better than later.

Looking at the code, it seems that ResolveReference will panic exactly when u == nil and ref.IsAbs() is false. So why not test that?

[wking]

Looking at the code, it seems that ResolveReference will panic exactly when u == nil and ref.IsAbs() is false.

I had to read it over a few times, but yeah, that looks right. Thanks :)

Cross-linking #20924 as a similar issue, but for different methods, in case anyone stumbles across the other while searching.