fix(ldf): seed from every compiled TU, and treat __has_include as undecidable - #1376
Merged
Merged
Conversation
…ecidable Closes #1337. Teensyduino bundled libraries landed on the include path but their sources never reached the link line, so any TU that included them compiled and then failed with `undefined reference`. All eight Teensy workflows red on FastLED master; the same class as #1214, which the symptom outlived. ## Local library sources are dependencies too `collect_project_seeds` skipped the `lib/` root wholesale, so a local library's own translation units were never scanned. FastLED expresses its Adafruit_NeoPixel and Audio dependencies exactly there — in `.cpp` files it compiles and links — and the resolver never saw them. The rule was "seeds are project translation units only", introduced by #1094 to stop an *inactive header* under `lib/` from self-selecting a framework library. That protection is intact: headers are still never seeds. What changes is the invariant it rested on, from "sketch only" to **what compiles is what seeds** — if the compiler builds a TU, its includes are real dependencies, and the scanner disagreeing with the compiler is the bug. Layout matters, and getting it wrong breaks the invariant the other way. An Arduino 1.5 library keeps sources in `src/`, a 1.0 library at the root, and neither compiles `examples/`, `extras/` or test trees. Seeding an example sketch would have the scanner claim dependencies the build never links — #1337 mirrored — so those directories are excluded and a test pins it. ## `__has_include` is undecidable, not false Nothing `#define`s a compiler builtin, so no macro set can settle it, and answering "false" is how an include the compiler *does* take stayed invisible. It is now Unknown: the arm is scanned, and the walker's own header resolution decides — which is the same question `__has_include` asks, so scanner and compiler agree by construction. Its argument is consumed too, along with any function-like macro's, so the `<` in `FL_HAS_INCLUDE(<SPI.h>)` is not parsed as a less-than. ## Unknown-ness combines per operand Found by the counterweight test rather than by reasoning. FastLED's current guard is `defined(FASTLED_USE_ADAFRUIT_NEOPIXEL) && FL_HAS_INCLUDE(<Adafruit_NeoPixel.h>)` — deliberately two signals. With a single global unknown flag the undecidable half poisoned the whole expression, so the library was selected even with the opt-in absent, putting sources on the link line that nothing references. `&&` now stays decidably false when either operand is decidably false, and `||` decidably true when either is decidably true, regardless of unknowns elsewhere. Both directions of the gate are tested. ## Also - `LDF_MODE_VERSION` 5→6: seeding semantics changed, so warm caches must not answer with the old rule. - `framework_libs.rs` was 998 LOC and would have crossed the 1000 gate; tests moved to `framework_libs_tests.rs` behind `#[path]`, the established pattern. No test content changed. - Doc comments updated where they still promised sketch-only seeding. ## Verified RED first: the reproduction fails on the old seeding, exactly as reported. Counterweights cover the mirrored risks — opt-in absent selects nothing, `examples/` never seeds, and #1094's inactive-header canary still passes. fbuild-build-engine 405, fbuild-header-scan 63, fbuild-library-select 31, workspace clippy `-D warnings`, full `dylint --all` sweep: all clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 6 minutes Limit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?Wait for the limit to reset, then comment An organization admin can change what happens after included review limits in Billing. How do review limits work?CodeRabbit enforces per-developer PR review limits within each organization. For paid Pro and Pro+ reviews, CodeRabbit uses a developer's included PR review attempts over the past 7 days to set the current hourly allowance. At typical activity levels, the full plan allowance applies. Higher sustained activity can lower the allowance until earlier attempts leave the 7-day window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
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
1 participant
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.
Closes #1337.
Two gaps, both between the scanner and the compiler
1. A local library's own sources were never scanned.
collect_project_seedsskipped thelib/root wholesale. FastLED expresses its Adafruit_NeoPixel and Audio dependencies in.cppfiles it compiles and links — the resolver never saw them, so the headers were on the include path and the sources never on the link line.The rule was "seeds are project translation units only", from #1094, to stop an inactive header under
lib/from self-selecting a framework library. That protection is intact — headers are still never seeds. What changes is the invariant underneath, from "sketch only" to what compiles is what seeds: if the compiler builds a TU, its includes are real dependencies, and the scanner disagreeing with the compiler is the bug your issue describes.Layout matters and getting it wrong breaks it the other way. Arduino 1.5 keeps sources in
src/, 1.0 at the root, and neither compilesexamples/,extras/or test trees. Seeding an example sketch would have the scanner claim dependencies the build never links — #1337 mirrored — so those are excluded, with a test pinning it.2.
__has_includeevaluated to false. Nothing#defines a compiler builtin, so no macro set can settle it. It's now undecidable: the arm is scanned, and the walker's own header resolution decides — the same question__has_includeasks, so the two agree by construction. Its argument is consumed as well (along with any function-like macro's), so the<inFL_HAS_INCLUDE(<SPI.h>)isn't parsed as a less-than.The counterweight test found a third thing
Your issue's guard has since become
defined(FASTLED_USE_ADAFRUIT_NEOPIXEL) && FL_HAS_INCLUDE(<Adafruit_NeoPixel.h>)— deliberately two signals. Writing the negative test (no opt-in → must not select) exposed that unknown-ness was a single global flag, so the undecidable half poisoned the whole expression and the library was selected even with the opt-in absent — sources on the link line that nothing references.&&now stays decidably false when either operand is decidably false, and||decidably true when either is decidably true, regardless of unknowns elsewhere. Both directions of the gate are tested. I'd have shipped that without the negative case.Verified
RED first — the reproduction fails on the old seeding, exactly as reported. Then the mirrored risks:
local_library_source_selects_a_framework_librarylocal_library_source_without_the_opt_in_selects_nothinglocal_library_examples_do_not_seedinactive_local_library_header_cannot_select_framework_libraryfbuild-build-engine 405, fbuild-header-scan 63, fbuild-library-select 31, workspace clippy
-D warnings, fulldylint --allsweep, platform-boundary — all clean locally.LDF_MODE_VERSION5→6, since seeding semantics changed and a warm cache must not answer with the old rule.framework_libs.rswas 998 LOC and would have crossed the 1000 gate, so its tests moved toframework_libs_tests.rsbehind#[path]— no test content changed.One honest caveat on the repro
FastLED has since gated the driver behind
FASTLED_USE_ADAFRUIT_NEOPIXEL, with a source comment citing #1214's sketch-only seeding as the reason. So the originalAdafruitBridgecommand may now take the fake-driver path consumer-side rather than failing to link. This PR fixes the fbuild-side gap regardless — and with the opt-in supplied, the library now selects, which is what makes the workaround unnecessary rather than load-bearing.🤖 Generated with Claude Code