CHORE: Replace parser.ParseDir, which is deprecated#4154
Closed
tlimoncelli wants to merge 1 commit intomainfrom
Closed
CHORE: Replace parser.ParseDir, which is deprecated#4154tlimoncelli wants to merge 1 commit intomainfrom
tlimoncelli wants to merge 1 commit intomainfrom
Conversation
philpennock
suggested changes
Mar 21, 2026
Contributor
philpennock
left a comment
philpennock
left a comment
There was a problem hiding this comment.
Basic smoke test: invoke go test -v -run TestCapabilitiesAreFiltered ./pkg/normalize
The reported details with this patch are very bogus. "is checked for with ..."
Before:
=== RUN TestCapabilitiesAreFiltered
capabilities_test.go:74: ok: providers.CanUseAKAMAICDN (4) is checked for with "AKAMAICDN"
capabilities_test.go:74: ok: providers.CanUseAKAMAITLC (29) is checked for with "AKAMAITLC"
capabilities_test.go:74: ok: providers.CanUseAlias (5) is checked for with "ALIAS"
capabilities_test.go:74: ok: providers.CanUseAzureAlias (6) is checked for with "AZURE_ALIAS"
capabilities_test.go:74: ok: providers.CanUseCAA (7) is checked for with "CAA"
capabilities_test.go:74: ok: providers.CanUseDHCID (8) is checked for with "DHCID"
capabilities_test.go:74: ok: providers.CanUseDNAME (9) is checked for with "DNAME"
capabilities_test.go:74: ok: providers.CanUseDNSKEY (24) is checked for with "DNSKEY"
capabilities_test.go:74: ok: providers.CanUseDS (10) is checked for with "DS"
capabilities_test.go:74: ok: providers.CanUseDSForChildren (11) is checked for with "DS"
capabilities_test.go:74: ok: providers.CanUseHTTPS (12) is checked for with "HTTPS"
capabilities_test.go:74: ok: providers.CanUseLOC (13) is checked for with "LOC"
capabilities_test.go:74: ok: providers.CanUseNAPTR (14) is checked for with "NAPTR"
capabilities_test.go:74: ok: providers.CanUseOPENPGPKEY (25) is checked for with "OPENPGPKEY"
capabilities_test.go:74: ok: providers.CanUsePTR (15) is checked for with "PTR"
capabilities_test.go:74: ok: providers.CanUseRP (17) is checked for with "RP"
capabilities_test.go:74: ok: providers.CanUseRoute53Alias (16) is checked for with "R53_ALIAS"
capabilities_test.go:74: ok: providers.CanUseSMIMEA (18) is checked for with "SMIMEA"
capabilities_test.go:74: ok: providers.CanUseSOA (19) is checked for with "SOA"
capabilities_test.go:74: ok: providers.CanUseSRV (20) is checked for with "SRV"
capabilities_test.go:74: ok: providers.CanUseSSHFP (21) is checked for with "SSHFP"
capabilities_test.go:74: ok: providers.CanUseSVCB (22) is checked for with "SVCB"
capabilities_test.go:74: ok: providers.CanUseTLSA (23) is checked for with "TLSA"
--- PASS: TestCapabilitiesAreFiltered (0.00s)
On this branch:
=== RUN TestCapabilitiesAreFiltered
capabilities_test.go:86: ok: providers.CanUseAKAMAICDN (0) is checked for with "AUTODNSSEC"
capabilities_test.go:86: ok: providers.CanUseAKAMAITLC (0) is checked for with "AUTODNSSEC"
capabilities_test.go:86: ok: providers.CanUseAlias (0) is checked for with "AUTODNSSEC"
capabilities_test.go:86: ok: providers.CanUseAzureAlias (0) is checked for with "AUTODNSSEC"
capabilities_test.go:86: ok: providers.CanUseCAA (0) is checked for with "AUTODNSSEC"
capabilities_test.go:86: ok: providers.CanUseDHCID (0) is checked for with "AUTODNSSEC"
capabilities_test.go:86: ok: providers.CanUseDNAME (0) is checked for with "AUTODNSSEC"
capabilities_test.go:86: ok: providers.CanUseDNSKEY (0) is checked for with "AUTODNSSEC"
capabilities_test.go:86: ok: providers.CanUseDS (0) is checked for with "AUTODNSSEC"
capabilities_test.go:86: ok: providers.CanUseDSForChildren (0) is checked for with "AUTODNSSEC"
capabilities_test.go:86: ok: providers.CanUseHTTPS (0) is checked for with "AUTODNSSEC"
capabilities_test.go:86: ok: providers.CanUseLOC (0) is checked for with "AUTODNSSEC"
capabilities_test.go:86: ok: providers.CanUseNAPTR (0) is checked for with "AUTODNSSEC"
capabilities_test.go:86: ok: providers.CanUseOPENPGPKEY (0) is checked for with "AUTODNSSEC"
capabilities_test.go:86: ok: providers.CanUsePTR (0) is checked for with "AUTODNSSEC"
capabilities_test.go:86: ok: providers.CanUseRP (0) is checked for with "AUTODNSSEC"
capabilities_test.go:86: ok: providers.CanUseRoute53Alias (0) is checked for with "AUTODNSSEC"
capabilities_test.go:86: ok: providers.CanUseSMIMEA (0) is checked for with "AUTODNSSEC"
capabilities_test.go:86: ok: providers.CanUseSOA (0) is checked for with "AUTODNSSEC"
capabilities_test.go:86: ok: providers.CanUseSRV (0) is checked for with "AUTODNSSEC"
capabilities_test.go:86: ok: providers.CanUseSSHFP (0) is checked for with "AUTODNSSEC"
capabilities_test.go:86: ok: providers.CanUseSVCB (0) is checked for with "AUTODNSSEC"
capabilities_test.go:86: ok: providers.CanUseTLSA (0) is checked for with "AUTODNSSEC"
--- PASS: TestCapabilitiesAreFiltered (1.69s)
PASS
Contributor
|
The second check is to edit If I apply this test of the test on main, then the test correctly fails when I comment out DNAME. If I do the same on this branch, then the test passes, which means it has been made ineffective. |
Contributor
|
See proposed replacement PR in #4195 |
Collaborator
|
closing so-as not to confuse with #4195 |
TomOnTime
pushed a commit
that referenced
this pull request
May 8, 2026
…4195) parser.ParseDir is deprecated since Go 1.25. Use packages.Load with NeedTypes and NeedTypesInfo to correctly extract CanUse* constant integer values via TypesInfo.Defs, preserving the test's ability to detect missing providerCapabilityChecks entries. --- This is a *replacement* PR for #4154. I took my review feedback from that PR, copied the markdown into two local files, and prompted Claude Code (using Sonnet 4.6) with: > Branch sx-pr/4154 contains one commit, which purports to "Replace parser.ParseDir, which is deprecated". But it is broken. This is a test of the codebase, via AST checks, that various providers.CanUse* checks are called appropriately. Files `local.pdp.review1.1` and `local.pdp.review1.2` (git-ignored) contain my review feedback, pointing out how the change is broken and how it can be validated. You are on a new git branch based off current main; please provide a correct replacement, and validate that it works. The next section is choice quotes from Claude's analysis while working, and the final section was Claude's summary at end-of-work. I have independently reviewed the code, it looks good, and I've validated that the tests look reasonable and correctly fail when a test is commented out (to ensure the AI did not fabricate that). I think this is a sane way forward. I thought it would be amusing to use Claude to critique Claude's earlier work. --- Now I understand the problem clearly. The broken PR sets `capabilityInts[name.Name] = 0` for all constants (the comment even admits it), which means every `CanUse*` constant maps to `int 0`, and since `capIntsToNames[0]` finds the first entry, all checks pass trivially. The test is completely neutered. The fix is to use `packages.NeedTypes`/`NeedTypesInfo` (already requested in the config) to actually read the integer values of the constants. Let me look at the providers package to understand the constants: [...] The problem is clear. The broken PR: 1. Uses `packages.Load` correctly to get type info (`NeedTypes`, `NeedTypesInfo`) 2. But then abandons the type info and sets all `capabilityInts` to `0`, making every `CanUse*` constant map to `CanAutoDNSSEC` (which is in `capIntsToNames[0]`), so all checks pass trivially The fix is to use `providers.TypesInfo.Defs` to look up the actual `*types.Const` value for each constant. Let me write the corrected test file: --- Both validations from the review pass: 1. Correct output: integer values are real (`(4)`, `(9)`, etc.) and each is paired with its correct record type — matches the "before" output exactly. 2. Test actually catches omissions: commenting out the DNAME entry causes `FAIL` with `MISSING: providers.CanUseDNAME (9)`. What was wrong in the PR: The code switched from `parser.ParseDir` to `packages.Load` correctly, but the author gave up on extracting real integer values and wrote `capabilityInts[name.Name] = 0` for everything. Since all constants got value `0`, they all matched `capIntsToNames[0] = AUTODNSSEC`, making the test trivially pass regardless of what was in `providerCapabilityChecks`. The fix: Instead of iterating the raw AST and guessing values, use `providers.TypesInfo.Defs` (which is what `NeedTypesInfo` provides) to look up each identifier's `*types.Const` object and call `constant.Int64Val(c.Val())` to get the actual iota integer. Co-authored-by: Phil's PT AI Bot Account <phil+ai@pennock-tech.com> Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
Fixed #4089
FYI: I used Claude to fix this. I actually don't understand the original code so please give this extra scrutiny.
CC @philpennock
Issue
lint is reporting that parser.ParseDir is deprecated:
Resolution
Use
golang.org/x/tools/go/packagesinstead.