Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

x/pkgsite: proposal for search filters: language features and possible compatibility issues #39195

Open
fgergo opened this issue May 21, 2020 · 13 comments

Comments

@fgergo
Copy link

@fgergo fgergo commented May 21, 2020

What is the URL of the page with the issue?

https://pkg.go.dev/

Proposal(?) to add search filter functionality to include/exclude features when searching for packages, modules.
Usefulness in descending order (for me at least):

  • cgo usage <-- this! ("cgo is not Go")
  • package unsafe usage
  • package reflect usage
  • package sync/atomic.*64* usage (to detect possible 32/64bit compatibility issues, anything else to detect possible issues?)
  • supported OS platforms (not sure how to check programmatically from source)
  • anything else?

Most probably these are only really useful if search filters are applicable to all dependencies transitively as well (except for the standard library of course).

for reference:
https://groups.google.com/forum/#!topic/golang-dev/U27D-g9j0LA

@gopherbot gopherbot added the pkgsite label May 21, 2020
@gopherbot gopherbot added this to the Unreleased milestone May 21, 2020
@julieqiu
Copy link
Contributor

@julieqiu julieqiu commented May 21, 2020

Thanks for filing this, @fgergo! Related: #37810 and #36952.

@JAicewizard
Copy link

@JAicewizard JAicewizard commented May 22, 2020

I dont think we should shame people for using reflect/unsafe.
We are nut the rust community, we can do better than shaming people for wanting something that is a bit faster. We saw what happened to actix-web, we should not encourage that in the go community.

If we'd rank packages by reflect encoding/json and encoding/gob would be all the way at the bottom, it is the nature of a language without first class code generation support has a high reflect usage in these packages.

@fgergo
Copy link
Author

@fgergo fgergo commented May 22, 2020

@JAicewizard I'm surprised you could read anything regarding "shaming people" from the proposal, I never intended to shame anybody. Sorry if you understood it like that.

Please consider searching on google.com for this:
-programming language

Results exclude all pages related to programming. The results include anything else related languages. #39195 is about a similar functionality.

Could you please help rephrase the proposal to exclude anything related shaming people?

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented May 22, 2020

Let's please take a step back and remember that the Gopher Code of Conduct (https://golang.org/conduct) asks us to be respectful and charitable in discussions.

We all want Go, and pkg.go.dev, to be as useful as possible.

Thanks.

@JAicewizard
Copy link

@JAicewizard JAicewizard commented May 22, 2020

@fgergo I didn't mean the intend is to shame people, but adding a metric that is not relevant will encourage, in one way or another, shaming. Nobody might say this out-loud, but things like "ew that package uses a lot reflect" will pop up into someones head. Nobody should pick one package over another just because one has a bit more unsafe than the other, as long as the API don't expose anything that is wrong.

Places where people DO start measuring things like unsafe as a measure of quality, like the rust community, people WILL get shamed for using unsafe/reflect, like what happened to actix-web.
By shaming I also didn't mean direct harassment, but also things like picking one package over another because of something irrelevant.

Other actual relevant things, like cgo, 32/64 bit incompatibility, OS compatibility etc should IMO be displayed/filtered on, they are things that should 100% influence your decision of one package over another, but reflect/unsafe don't belong in that list.

Sorry if it felt like a personal attack, it was not meant that way. We should try and avoid actix-web in go, and exposing stats about irrelevant, but possibly with negative reputation, is a step towards an actix-web situation.

@JAicewizard
Copy link

@JAicewizard JAicewizard commented May 22, 2020

@ianlancetaylor I think my point is clear, and I hope my intend it to avoid a toxic community, not encourage it, is also somewhat clear.

I think we shouldn't continue to discuss my exact words, I hope my point is clear, sorry if I went a bit far in trying to make it clear. Again sorry if it felt like I was being disrespectfull to @fgergo.

@randall77
Copy link
Contributor

@randall77 randall77 commented May 22, 2020

I think we do want to discourage use of unsafe, at least to a degree. Avoiding unsafe eliminates a wide variety of possible bugs. Of course, bugs can be avoided other ways (more tests, fuzzing, etc.) so as with any signal, it should probably not stand alone.

If there are two packages that do the same thing, everything else being equal I would choose the one that didn't use unsafe.

@JAicewizard
Copy link

@JAicewizard JAicewizard commented May 22, 2020

Yes, but things will never be equal, otherwise the unsafe package wouldn't exist.
If the intend is to avoid bugs a better rating would be amount of warning from static-check since I suspect most bugs don't come from unsafe usage.
Filtering based on unsafe wont just discourage it. Unsafe is almost always used when it brings a significant performance improvement, if it isn't necessary nobody would use it. But sometimes it is necessary and we shouldn't judge a package by its unsafe unsafe. (unless it is significantly slower than a "safe" alternative, then judge away)

edit:
I would also like to add that you cant just use unsafe LOC as a measure, some things are super unsafe, some things depend on alignment, and some things just want to make a byte-slice a string because they know that the slice isn't written to and the only reference to it would go out of scope the very next line. how unsafe unsafe code is depends on who you ask, what the context is, and if the unsafe is explicitly tested for or not.
I would definitely not use a package that had as a requirement "don't edit this byte-slice please things will break", but if the author made sure that would not be necessary, I would not hesitate.

@randall77
Copy link
Contributor

@randall77 randall77 commented May 22, 2020

If the intend is to avoid bugs a better rating would be amount of warning from static-check since I suspect most bugs don't come from unsafe usage.

Sure, that would be a good signal also. Probably a better signal. But we don't have to choose just one.

Unsafe is almost always used when it brings a significant performance improvement, if it isn't necessary nobody would use it. But sometimes it is necessary and we shouldn't judge a package by its unsafe unsafe. (unless it is significantly slower than a "safe" alternative, then judge away)

If I'm looking for a package which is performance-critical, I would absolutely consider unsafe packages, especially if their performance was better than alternatives.
On the other hand, if I'm looking for something which isn't performance-critical, I would avoid unsafe packages unless there was no alternative (or the alternatives were bad for other reasons).

@JAicewizard
Copy link

@JAicewizard JAicewizard commented May 22, 2020

I think the intend is to avoid buggy code, and amount of unsafe usage is not a good way to detect buggy code. Another better metric would be how populair it is or how well maintained it is, something that is very badly maintained should be avoided because a bug is unlikely to be resolved, while a good maintained popular package is likely to be very robust.
By default you should always look for the community default, since that is probably easiest to use and best tested unsafe or not. In that way I would actually suggest filtering by downloads or some other popularity metric.

Putting a direct metric on unsafe/reflect usage will at some point create a actix-web situation, I think we should try and avoid that at all cost and that was my initial reason for not wanting this feature.

@fgergo
Copy link
Author

@fgergo fgergo commented May 22, 2020

A few meta-notes:
Please note, English is not my primary language.
If I wrote something not charitable or not respectful, please help me correct that. I still don't get what is not clear in my original proposal. (Please note: wording in #39195 is different than in my original email on the mailing list.)

Maybe since I'd partly socialized on c.o.l.a in the end of the 90s, I still don't see how anything @JAicewizard wrote would be personal or disrespectful.

Rereading everything I wrote above I am still not sure a github issue comment thread is the best place to have this conversation. Should I just delete this comment?

On the technical merit of #39195:
I am not proposing to assign an objective or subjective value to any module or package.

I am proposing something like this: when I type this in the search box:
video processing -i"syscall" -i"fmt" +i"unsafe"
I'd expect a list of modules relevant with video processing and which are not importing syscall and fmt, but importing unsafe.

If somebody is not interested on any specific package usage, they just won't use this feature of search.

@JAicewizard
Copy link

@JAicewizard JAicewizard commented May 22, 2020

Although I definitely still don't agree, having something to filter out any package for me doesn't sound anywhere near as bad as singling out just unsafe/reflect.
I still don't see the relevance but if it is made generally available for all imports that sounds a lot less like it is trying to pinn down on one single import and having it be a no import flag, rather than a threshold also avoids the actix-web situation.
Everyone has been able to read my 2 cents now, even though they are mostly irrelevant at this point, so I leave it to the go.pkg.dev maintainers to decide.

@fgergo I don't see what would have been insulting, but Ian posted the coc so I felth like maybe it could be seen that way by someone else.

Sorry i had to edit this, I'm on my phone now and accidentally pressed the comment button

@fgergo
Copy link
Author

@fgergo fgergo commented May 22, 2020

Edit2:
Sorry, can't do what gofmt -r does, since parser.ParseExpr() won't parse declarations of course. Need to invent new mini language.
Anybody?

@julieqiu julieqiu changed the title go.dev: proposal for search filters: language features and possible compatibility issues x/pkgsite: proposal for search filters: language features and possible compatibility issues Jun 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
6 participants
You can’t perform that action at this time.