New*Fs functions should return the concrete type

[bep]

Having to do:

 NewBasePathFs(fs, baseDir).(*BasePathFs)

Doesn't make much sense. I don't see the problem going the other way around; but as this is a file system with a very specific contract, I want to know what it is.

[vetinari]

you mean like this?

@@ -22,7 +22,7 @@ type BasePathFs struct {
        path   string
 }

-func NewBasePathFs(source Fs, path string) Fs {
+func NewBasePathFs(source Fs, path string) *BasePathFs {
        return &BasePathFs{source: source, path: path}
 }

[bep]

Yes. Not sure why it is a pointer, but that is another debate.

[mbertschler]

This was introduced in a commit https://github.com/spf13/afero/commit/fe8e8953368bff0a84babecf4e2a6c965c0453c8 where @spf13 added a lot of New*Fs() function, and most of them return Fs. I also think the concrete type would be more useful in this case, as people can just assign it to a var like var fs afero.Fs = afero.NewBasePathFs(). I believe the pointer here is not needed, but only for mutable things like MemMapFs. Probably every of the New*Fs should return the concrete type.

[bep]

I saw a discussion on gopher-nuts about this, and I believe one of the Go core devs argued that functions/methods should take interfaces as arguments when possible, but return concrete types. I agree.

[mbertschler]

Makes sense, because this is the most flexible way. If the argument is the interface, you can pass the interface or concrete value, and if the return is the concrete type you can assign it to an interface or concrete variable, all without any assertions.

[xiegeo]

Is there a reason why this is not of high priority? Seems like an easy fix, and high impact as it changes the public signature.

[moorereason]

I disagree with this issue principally because of the rules of generality in Effective Go. Read it since I won't repeat the argument.

BasePathFs and MemMapFs are the only Fs implementations with an extra method: RealPath and List, respectively. BasePathFs.RealPath should not be exported in the first place -- that's what Stat is for. MemMapFs.List looks like a debugging feature that most people are likely not using; I'd suggest unexporting or removing List.

Since all Fs implementations should be exporting the same methods, there's no need to export a concrete type. Everything we need is in the interface.

[bep]

@moorereason to get some effect out of what you're saying, you need to un-export the concrete types, which would be painful in the wild.

My main argument for returning the concrete type here is the case of afero.BasePathFs. This is security related; I really want to know that it is this special kind of restricted FS. Currently I could do a type assertion to check, which i could not do if the type was unexported, and then you might as well just return it.

[moorereason]

I'm not saying that BasePathFs itself should not be exported. It is and should be. I only mean that BasePathFs.RealPath should be unexported. RealPath adds a method that's already wrapped by Stat.

If you are concerned about the type of the underlying Fs implementation, a type assertion is the way to go.

[bep]

I only mean that BasePathFs.RealPath should be unexported.

Sure, but that is a topic for another thread.

The "effective Go" link you provide seems to be old. I have seen more recent comments by core Go people that evangelise "interface in, type out". There are, of course, exceptions to this rule, most often when you don't care/know about the type. With file systems, you often care about what it really is, and the API gets much clearer when you return the concrete type: If you need to use it as an interface, just use it as is, if you need the concrete type, no need to to do magic type casting: It is obvious in type func signature what you get.

[bep]

An added note to the above: I have a long background with Java, and I kind of hate when people talk about idomatic Go -- but when it comes to interfaces, you will have to think differently for it to be really effective.

[jxsl13]

imo filesystem abstractions may provide more than just what the Fs interface has to offer. In my case rollback mechanics of filesystem modifications.

You can basically put implementations into functions requiring the interface but getting the implementations back from an interface is imo a giant hassle.