bazel: detect go standard library deprecation errors #87327
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
This change is |
rickystewart
approved these changes
Sep 6, 2022
|
@healthy-pod This may warrant a bug report upstream. It's known that stdlib packages don't always have analysis result and I think this check should still work even in that scenario. Best case scenario we might be able to remove this custom patch in the future. |
|
bors r=rickystewart |
|
🕐 Waiting for PR status (Github check) to be set, probably by CI. Bors will automatically try to run when all required PR statuses are set. |
|
This is missing the CLA approval. Need to get that done before the PR can be processed. |
Right thanks for pointing that out. I merged PRs before though so trying to find out why |
|
Sometimes the webhook doesn't fire. |
|
bors r- |
Previously, `staticcheck` only raised deprecation errors from non-stdlib packages because the analysis pass used by the deprecation analyzer is created by `nogo` which doesn't analyze the standard library packages. This code change patches `staticcheck` to let it detect deprecation errors from the standard library and raise them. It also removes two uses of `tls.Config.PreferServerCipherSuites` because it's deprecated since Go 1.18 and is a legacy field that is ignored and has no effect. Closes cockroachdb#84877 Release justification: Non-production code change Release note: None
healthy-pod
force-pushed
the
fix-nogo-stdlib-deprecations
branch
from
04e574e to
90a0126
Compare
|
you can issue the bors command now. no need to wait for CI |
|
bors r=rickystewart |
|
Stopped waiting for PR status (Github check) without running due to duplicate requests to run. You may check Bors to see that this PR is included in a batch by one of the other requests. |
|
Build failed (retrying...): |
|
Build succeeded: |
healthy-pod
pushed a commit
to healthy-pod/cockroach
that referenced
this pull request
Dec 1, 2022
In cockroachdb#87327, we patched `staticcheck` to detect deprecated "objects" from the standard library. This patch ensures that we also detect deprecated "packages". Closes cockroachdb#84877 Release note: None
craig bot
pushed a commit
that referenced
this pull request
Dec 2, 2022
92713: sql: fix left semi and left anti virtual lookup joins r=yuzefovich a=yuzefovich This commit fixes the execution of the left semi and left anti virtual lookup joins. The bug was that we forgot to project away the looked up columns (coming from the "right" side) which could then lead to wrong columns being used higher up the tree. The bug was introduced during 22.1 release cycle where we added the optimizer support for generating plans that could contain left semi and left anti virtual lookup joins. This commit fixes that issue as well as the output columns of such joins (I'm not sure whether there is a user facing impact of having incorrect "output columns"). Additionally, this commit fixes the execution of these virtual lookup joins to correctly return the input row only once. Previously, for left anti joins we'd be producing an output row if there was a match (which is wrong), and for both left semi and left anti we would emit an output row every time there was a match (but this should be done only once). (Although I'm not sure whether it is possible for virtual indexes to result in multiple looked up rows.) Also, as a minor simplification this commit makes it so that the output rows are not added into the row container for left semi and left anti and the container is not instantiated at all. Fixes: #91012. Fixes: #88096. Release note (bug fix): CockroachDB previously could incorrectly evaluate queries that performed left semi and left anti "virtual lookup" joins on tables in `pg_catalog` or `information_schema`. These join types can be planned when a subquery is used inside of a filter condition. The bug was introduced in 22.1.0 and is now fixed. 92783: nogo: detect deprecated go standard library packages r=rickystewart a=healthy-pod In #87327, we patched `staticcheck` to detect deprecated "objects" from the standard library. This patch ensures that we also detect deprecated "packages". Closes #84877 Release note: None 92859: rowcontainer: address an old TODO r=yuzefovich a=yuzefovich This commit addresses an old TODO about figuring out why we cannot reuse the same buffer in `diskRowIterator.Row` method. The contract of that method was previously confusing - it says that the call to `Row` invalidates the row returned on the previous call; however, the important piece was missing - that the datums themselves cannot be mutated (this is what we assume elsewhere and perform only the "shallow" copies). This commit clarifies the contract of the method and explicitly explains why we need to allocate fresh byte slices (which is done via `ByteAllocator` to reduce the number of allocations). Additional context can be found in #43145 which added this copy in the first place. Here is a relevant quote from Alfonso: ``` I think what's going on here is that this type of contract (you may only reuse the row until the next call) is a bit unclear. `CopyRow`s of `EncDatum`s are only implemented as shallow copies, so the reference to this reuse only applies to the `EncDatumRow`, but not to the `encoded` field, which is what is rewritten in this case. The `encoded` field will not be copied, so I think the tests are probably failing due to that. This is unfortunate and there doesn't seem to be a good reason for it. To implement deep copies, we will have to implement deep copies for `Datum`s as well. ``` I think we were under the impression that we'd implement the "deep copy" in `CopyRow`, but I highly doubt we'll do so given that we're mostly investing in the columnar infrastructure nowadays, and the current way of performing shallow copying has worked well long enough. Epic: None Release note: None Co-authored-by: Yahor Yuzefovich <[email protected]> Co-authored-by: healthy-pod <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Previously,
staticcheckonly raised deprecation errorsfrom non-stdlib packages because the analysis pass used
by the deprecation analyzer is created by
nogowhich doesn'tanalyze the standard library packages.
This code change patches
staticcheckto let it detect deprecationerrors from the standard library and raise them. It also removes two
uses of
tls.Config.PreferServerCipherSuitesbecause it's deprecatedsince Go 1.18 and is a legacy field that is ignored and has no effect.
Closes #84877
Release justification: Non-production code change
Release note: None