[Author Prev][Author Next][Thread Prev][Thread Next][Author Index][Thread Index]
Re: [tor-bugs] #31794 [Circumvention/Snowflake]: Errors swallowed
#31794: Errors swallowed
-------------------------------------+------------------------------
Reporter: sah | Owner: (none)
Type: defect | Status: needs_review
Priority: Medium | Milestone:
Component: Circumvention/Snowflake | Version:
Severity: Normal | Resolution:
Keywords: | Actual Points:
Parent ID: | Points:
Reviewer: cohosh | Sponsor:
-------------------------------------+------------------------------
Comment (by dcf):
Thanks for this report. I've only looked at it for a moment. May of
golint's suggestions are worth doing something about, but I'm skeptical of
others.
I don't think we necessarily want to make the code completely lint-free.
The [https://github.com/golang/lint#purpose upstream documentation] even
says, "The suggestions made by golint are exactly that: suggestions. We
will not be adding pragmas or other knobs to suppress specific warnings,
so do not expect or require code to be completely 'lint-free'." golint
seems like it's more useful as a code review tool, not something to
enforce across the entire codebase. (Unlike `go vet`, which is already
part of our CI.)
What happens if you run the linter on the Go stdlib (or part of it)? It
would be good to know what suggestions often turn up in well-written Go
code.
{{{
snowflake.go:82:20: error strings should not be capitalized or end with
punctuation or a newline (golint)
return fmt.Errorf("SetDeadline not implemented")
^
snowflake.go:86:20: error strings should not be capitalized or end with
punctuation or a newline (golint)
return fmt.Errorf("SetReadDeadline not implemented")
^
snowflake.go:90:20: error strings should not be capitalized or end with
punctuation or a newline (golint)
return fmt.Errorf("SetWriteDeadline not implemented")
^
}}}
Clearly these reports are wrong, because the error message starts with an
identifier. I see
[https://github.com/shaneHowearth/snowflake/commit/4d57e9dcd44d05d63c8945e76c31395cce902112
#diff-ba4bd8a4477426567c409d66c2cf8a28 here], for example, they are
exempted with `nolint: golint`; but
[https://github.com/shaneHowearth/snowflake/compare/linter-fixes#diff-
e32e5d12043e17d487f1252ee61dfd5f here] the error message changes so it no
longer matches the identifier. Actually, we could probably sidestep the
whole issue by just changing all these messages to "not implemented", or
using a single global instance of `var errNotImplemented error =
errors.New("not implemented")`.
{{{
snowflake.go:230:24: should drop = 0 from declaration of var numHandlers;
it is the zero value (golint)
var numHandlers int = 0
^
}}}
I don't think it helps understanding to drop the explicit zero value here.
{{{
snowflake.go:142:7: shadow: declaration of "err" shadows declaration at
line 116 (govet)
n, err := pw.Write(msg)
^
}}}
This shadowing looks like deliberate limiting of variable scope. At least
when I write something like this, it's intentional. I ''want'' `err` in
the inner scope not to affect any other variable that happens to be called
`err` in the outer scope. I think the most likely case where this is an
actual problem is if `err` is a named return parameter.
--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/31794#comment:13>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online
_______________________________________________
tor-bugs mailing list
tor-bugs@xxxxxxxxxxxxxxxxxxxx
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs