proposal: net/url: add URL.Clone method

[icholy]

Copying a url is often done by shallow copying the struct. This can be confusing because it's not clear if the Userinfo should also be copied.

u, _ := url.Parse("http://foo")

// shallow copy the struct
u2 := *u

Another option is to format/re-parse the url. But this adds unnecessary overhead.

u2, _ := url.Parse(u.String())

Given how often url cloning comes up, I think it warrants adding a helper.

[icholy]

Related #38351

[ianlancetaylor]

What new API do you suggest?

[ianlancetaylor]

That is, what is the exact definition of the new Clone method?

[icholy]

@ianlancetaylor

// Clone returns a copy of u.
func (u *URL) Clone() *URL {
    u2 := *u
    return &u2
}

[rsc]

I'm confused about this. This operation would apply to many structs where you want to make a change to one field to produce a new copy. This ends up being a common idiom in Go. Why is URL special? Why does it merit a special method?

You wrote:

Given how often url cloning comes up, I think it warrants adding a helper.

But honestly I have not seen it come up often at all, which would warrant not adding a helper.

[icholy]

@rsc

Why is URL special? Why does it merit a special method?

The Userinfo field makes it unclear if the u2 := *u approach is correct.

But honestly I have not seen it come up often at all, which would warrant not adding a helper.

I have no empirical evidence to support my statement. However, I like to use this type of pattern:

// will usually come from configuration
base, _ := url.Parse("http://foo/bar")

func DoReq() error {
  u := base.Clone()
  u.Path = path.Join(u.Path, "my/route")

  // use the url to make a request
  // ...
}

[earthboundkid]

I also end up making modifications to a base URL (e.g. here). Maybe it's enough just to document that URLs can be shallow copied safely?

[rsc]

@carlmjohnson "Can be shallow copied safely" is true of many types, not just URLs. But also, URLs can be mutated safely too, if you own the URL. Just like most types.

[earthboundkid]

@carlmjohnson "Can be shallow copied safely" is true of many types, not just URLs.

Yes but as @icholy said originally,

This can be confusing because it's not clear if the Userinfo should also be copied.

You have to dig into the documentation for UserInfo to see that it’s immutable.

[rsc]

You have to dig into the documentation for UserInfo to see that it’s immutable.

But the reason to make a shallow copy is to get a local copy you can mutate. If you are not going to modify the UserInfo in the copy then you don't need to dig into the documentation.

[rsc]

It seems like there are two concerns with this proposal:

1. How often is this really needed? If not much, then we shouldn't add new API.
2. When it is needed, why isn't doing a u2 := *u, modify u2 the answer that people should reach for? Why is a new method warranted? (Are there uses other than preparing to modify a URL?)

[ainar-g]

Regarding the first question, I've definitely seen the “base URL clone plus path” pattern described above in the wild. As for the second, perhaps this is a documentation issue, and we just need to add an example of doing that to the docs?

[icholy]

I'd be happy with an example or a note in the documentation.

[rsc]

I still don't understand what exactly is worth calling out in URL's documentation.
It is a general property of data that if you want to make a copy before mutating you can do u2 := *u; mutate u2.

And I still don't understand how often this operation is needed on URLs.
@ainar-g has seen it, but that establishes existence not frequency.

[rsc]

Put another way, if we decide to add net.URL.Clone, what other types will we need to add Clone to?
It kind of seems like we'd need to add it to almost any type in the standard library.
Is there something special about URL that I am missing?

[earthboundkid]

Speaking for myself, I’m happy not to have a Clone() method, but just noting that even though UserInfo is a pointer, it’s immutable so you don’t have to worry about it being mutated by someone else holding the original url.URL.

[wader]

// will usually come from configuration
base, _ := url.Parse("http://foo/bar")

func DoReq() error {
  u := base.Clone()
  u.Path = path.Join(u.Path, "my/route")

  // use the url to make a request
  // ...
}

For me in most these cases what i actually want is to resolve a new URL. So in this case if I understand your problem correctly I would do

base, _ := url.Parse("http://foo/bar/")
u := base.ResolveReference(&url.URL{Path: "my/route"})

Maybe a bit cumbersome but I like that it makes it very explicit what is going on.

[icholy]

@wader I don't like ResolveReference because it requires the base url to have a trailing slash. https://play.golang.org/p/oka6MJ1674i

@rsc IMO the ambiguity around UserInfo is the "special" thing about it. However, I agree that the Clone method doesn't have a precedent here. I'm closing this issue in favour of #38351