[tor-bugs] #31794 [Circumvention/Snowflake]: Errors swallowed

[Tor Bug Tracker & Wiki]

#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