https://pantsbuild.org/ logo
f

fast-nail-55400

11/04/2021, 7:08 PM
@hundreds-father-404: what are your thoughts on whether or not first-party package analysis should fail due to syntax errors? my inclination is that it should not fail; later compilation will fail but package analysis shouldn’t. (context is that first_party_pkg_test has a unit test that looks for failure of analysis due to a syntax error in a go file)
h

hundreds-father-404

11/04/2021, 7:11 PM
It should behave how
FallibleFirstPartyPkgInfo
was before: gracefully handle the error. Don't crash Pants, but also indicate it was a failure so that callers can decide what to do
check
will output it as a failure, for example.
package
should crash and burn
f

fast-nail-55400

11/04/2021, 7:13 PM
but
check
will try to compile it and the failure will be the compile failure
but the analysis itself should not fail
h

hundreds-father-404

11/04/2021, 7:15 PM
Nack,
check
will not compile if analysis failed. Check out
go/goals/check.py
f

fast-nail-55400

11/04/2021, 7:17 PM
context is my PR to add go:embed comment scanning. my point is that if that PR changes the behavior to allow analysis to succeed, then
check
will try and compile
the question of this thread is whether that semantic change in that PR is agreeable
h

hundreds-father-404

11/04/2021, 7:20 PM
Oh, I don't see the benefit? Perhaps that the error message would be better coming from Go rather than from our own parser? It does simplify the code a little if we have
FirstPartyPkgInfo
always be infallabile, altho that isn't reason enough imo to make the semantic change
f

fast-nail-55400

11/04/2021, 7:22 PM
the semantic change is part of adopting the rules_go parsing code as-is
h

happy-kitchen-89482

11/04/2021, 7:22 PM
I tend to agree that dep inference shouldn't fail on parse errors (a warning is appropriate)
the user will expect to get those kinds of errors from the compiler
we shouldn't get ahead of that
👍 1
f

fast-nail-55400

11/04/2021, 7:24 PM
one issue is that the import extraction could have failed due to the parse error
so analysis will be “incomplete” in that case
h

hundreds-father-404

11/04/2021, 7:26 PM
Dep inference has that exact behavior right now, Benjy I think the only impact of Tom's change would be: • Dep inference no longer warns when there was a parse error •
check
,
test
, and
package
attempt to compile bad packages, rather than early aborting Does that sound right?
f

fast-nail-55400

11/04/2021, 7:28 PM
that sounds right.
although I can have the package analysis flag the parse error in a field and the Go rules can still warn in that case
h

hundreds-father-404

11/04/2021, 7:30 PM
That sounds like what is already happening with
FallibleFirstPartyPkgInfo
?
f

fast-nail-55400

11/04/2021, 8:00 PM
yeah and was easy enough to keep the existing semantics
👍 1
h

happy-kitchen-89482

11/04/2021, 9:49 PM
Yes, that sounds right to me. I think that's what people will expect.
1