Skip to content

fix: resolve compiler warnings and add CI warning detection #76

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
loothero merged 2 commits into from
Mar 8, 2026
Merged

fix: resolve compiler warnings and add CI warning detection #76

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
39c4ead
fix: resolve compiler warnings and add CI warning detection
loothero Mar 8, 2026
09140e0
fix: remove remaining unused Fixed import in math/tests
loothero Mar 8, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 19 additions & 4 deletions .github/workflows/main-ci.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -140,8 +140,14 @@ jobs:
- name: Warm snforge plugin cache
run: snforge --version

- name: Verify workspace compiles
run: scarb build --workspace
- name: Verify workspace compiles (no warnings)
run: |
scarb build --workspace 2>&1 | tee /tmp/build.log
if grep -qE '^ --> .*\.cairo:[0-9]+:[0-9]+' /tmp/build.log; then
echo "::error::Build produced compiler warnings. Fix all warnings before merging."
grep -B1 -E '^ --> .*\.cairo:[0-9]+:[0-9]+' /tmp/build.log
exit 1
fi

test:
name: test (${{ matrix.module }})
Expand Down Expand Up @@ -284,9 +290,18 @@ jobs:
- name: Run tests with coverage
run: |
if [ "${{ matrix.package }}" = "game_components_${{ matrix.module }}" ]; then
snforge test -p ${{ matrix.package }} --fuzzer-runs ${{ matrix.fuzzer_runs }} --coverage
snforge test -p ${{ matrix.package }} --fuzzer-runs ${{ matrix.fuzzer_runs }} --coverage 2>&1 | tee /tmp/test.log
else
snforge test -p ${{ matrix.package }} "::${{ matrix.module }}::" --fuzzer-runs ${{ matrix.fuzzer_runs }} --coverage
snforge test -p ${{ matrix.package }} "::${{ matrix.module }}::" --fuzzer-runs ${{ matrix.fuzzer_runs }} --coverage 2>&1 | tee /tmp/test.log
fi

- name: Check for compiler warnings
if: always() && steps.cache-tools.outcome != 'failure'
run: |
if [ -f /tmp/test.log ] && grep -qE '^ --> .*\.cairo:[0-9]+:[0-9]+' /tmp/test.log; then
echo "::error::Test compilation produced warnings. Fix all warnings before merging."
grep -B1 -E '^ --> .*\.cairo:[0-9]+:[0-9]+' /tmp/test.log
Comment on lines +301 to +303
Copy link

Copilot AI Mar 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The warning detector is keyed only on source-location lines (--> *.cairo:line:col), which are also emitted for compilation errors. Because this step runs with if: always(), a compile failure can be misreported as “warnings” (and potentially add noise on top of the real failure). Consider narrowing the grep to actual warning headers (e.g., match warning: and then include the following location lines), or gate this step to only run when the test command succeeded.

Suggested change
if [ -f /tmp/test.log ] && grep -qE '^ --> .*\.cairo:[0-9]+:[0-9]+' /tmp/test.log; then
echo "::error::Test compilation produced warnings. Fix all warnings before merging."
grep -B1 -E '^ --> .*\.cairo:[0-9]+:[0-9]+' /tmp/test.log
if [ -f /tmp/test.log ] && grep -q 'warning:' /tmp/test.log; then
echo "::error::Test compilation produced warnings. Fix all warnings before merging."
grep -n -B2 -A2 'warning:' /tmp/test.log

Copilot uses AI. Check for mistakes.
exit 1
fi

- name: Upload coverage to Codecov
Expand Down
23 changes: 19 additions & 4 deletions .github/workflows/pr-ci.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -362,8 +362,14 @@ jobs:
- name: Warm snforge plugin cache
run: snforge --version

- name: Verify workspace compiles
run: scarb build --workspace
- name: Verify workspace compiles (no warnings)
run: |
scarb build --workspace 2>&1 | tee /tmp/build.log
if grep -qE '^ --> .*\.cairo:[0-9]+:[0-9]+' /tmp/build.log; then
echo "::error::Build produced compiler warnings. Fix all warnings before merging."
grep -B1 -E '^ --> .*\.cairo:[0-9]+:[0-9]+' /tmp/build.log
exit 1
fi

test:
name: test (${{ matrix.module }})
Expand Down Expand Up @@ -434,9 +440,18 @@ jobs:
- name: Run tests with coverage
run: |
if [ "${{ matrix.package }}" = "game_components_${{ matrix.module }}" ]; then
snforge test -p ${{ matrix.package }} --fuzzer-runs ${{ matrix.fuzzer_runs }} --coverage
snforge test -p ${{ matrix.package }} --fuzzer-runs ${{ matrix.fuzzer_runs }} --coverage 2>&1 | tee /tmp/test.log
else
snforge test -p ${{ matrix.package }} "::${{ matrix.module }}::" --fuzzer-runs ${{ matrix.fuzzer_runs }} --coverage
snforge test -p ${{ matrix.package }} "::${{ matrix.module }}::" --fuzzer-runs ${{ matrix.fuzzer_runs }} --coverage 2>&1 | tee /tmp/test.log
fi

- name: Check for compiler warnings
if: always() && steps.cache-tools.outcome != 'failure'
run: |
if [ -f /tmp/test.log ] && grep -qE '^ --> .*\.cairo:[0-9]+:[0-9]+' /tmp/test.log; then
echo "::error::Test compilation produced warnings. Fix all warnings before merging."
grep -B1 -E '^ --> .*\.cairo:[0-9]+:[0-9]+' /tmp/test.log
Comment on lines +449 to +453
Copy link

Copilot AI Mar 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The warning detector is keyed only on source-location lines (--> *.cairo:line:col), which are also emitted for compilation errors. Because this step runs with if: always(), a compile failure can be misreported as “warnings” (and potentially add noise on top of the real failure). Consider narrowing the grep to actual warning headers (e.g., match warning: and then include the following location lines), or gate this step to only run when the test command succeeded.

Suggested change
if: always() && steps.cache-tools.outcome != 'failure'
run: |
if [ -f /tmp/test.log ] && grep -qE '^ --> .*\.cairo:[0-9]+:[0-9]+' /tmp/test.log; then
echo "::error::Test compilation produced warnings. Fix all warnings before merging."
grep -B1 -E '^ --> .*\.cairo:[0-9]+:[0-9]+' /tmp/test.log
if: success() && steps.cache-tools.outcome != 'failure'
run: |
if [ -f /tmp/test.log ] && grep -q '^warning:' /tmp/test.log; then
echo "::error::Test compilation produced warnings. Fix all warnings before merging."
grep -A1 -E '^warning:' /tmp/test.log

Copilot uses AI. Check for mistakes.
exit 1
fi

- name: Upload coverage to Codecov
Expand Down
2 changes: 1 addition & 1 deletion packages/interfaces/src/minigame/objectives.cairo
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
// Minigame objectives extension interface
use crate::structs::minigame::{GameObjective, GameObjectiveDetails};
use crate::structs::minigame::GameObjectiveDetails;

/// SNIP-5 interface ID derived via src5_rs: XOR of extended function selectors
/// - objective_exists(u32)->E((),())
Expand Down
2 changes: 1 addition & 1 deletion packages/metagame/src/entry_fee/tests/mocks/entry_fee_mock.cairo
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ pub mod EntryFeeMock {

#[external(v0)]
fn set_entry_fee(ref self: ContractState, context_id: u64, entry_fee: EntryFee) {
self.entry_fee.set_entry_fee(context_id, entry_fee);
let _ = self.entry_fee.set_entry_fee(context_id, entry_fee);
}
Comment on lines 39 to 42
Copy link

@gemini-code-assist gemini-code-assist bot Mar 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

While let _ = ... silences the compiler warning, it's generally better to propagate #[must_use] return values, especially in a test mock. The underlying set_entry_fee function returns an Option<EntryFeeConfig>, which could be valuable for assertions in your tests.

By modifying the function to return this value, the mock becomes more transparent and robust. You'll also need to add EntryFeeConfig to your imports from crate::entry_fee::structs.

    #[external(v0)]
    fn set_entry_fee(ref self: ContractState, context_id: u64, entry_fee: EntryFee) -> Option<EntryFeeConfig> {
        self.entry_fee.set_entry_fee(context_id, entry_fee)
    }


#[external(v0)]
Expand Down
6 changes: 2 additions & 4 deletions packages/metagame/src/entry_requirement/entry_requirement_component.cairo
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,9 +10,7 @@
pub mod EntryRequirementComponent {
use core::num::traits::Zero;
use game_components_interfaces::entry_requirement::{IENTRY_REQUIREMENT_ID, IEntryRequirement};
use interfaces::entry_requirement_extension::{
IENTRY_REQUIREMENT_EXTENSION_ID, IEntryValidatorDispatcher, IEntryValidatorDispatcherTrait,
};
use interfaces::entry_requirement_extension::IENTRY_REQUIREMENT_EXTENSION_ID;
use openzeppelin_interfaces::erc721::IERC721_ID;
use openzeppelin_interfaces::introspection::{ISRC5Dispatcher, ISRC5DispatcherTrait};
use openzeppelin_introspection::src5::SRC5Component;
Expand All @@ -29,7 +27,7 @@ pub mod EntryRequirementComponent {
use crate::entry_requirement::store::Store;
use crate::entry_requirement::structs::{
EntryRequirement, EntryRequirementMeta, EntryRequirementMetaStorePacking,
EntryRequirementType, ExtensionConfig, QualificationEntries, QualificationProof,
EntryRequirementType, QualificationEntries, QualificationProof,
};

#[storage]
Expand Down
1 change: 0 additions & 1 deletion packages/metagame/src/prize/structs.cairo
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -280,7 +280,6 @@ pub impl CustomSharesImpl of CustomSharesTrait {

#[cfg(test)]
mod packed_erc20_data_tests {
use starknet::storage_access::StorePacking;
use super::{PackedERC20Data, PackedERC20DataPacking};

// -------------------------------------------------------------------------
Expand Down
2 changes: 1 addition & 1 deletion packages/metagame/src/ticket_booth/ticket_booth_component.cairo
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ pub mod TicketBoothComponent {
StoragePointerWriteAccess,
};
use starknet::{get_block_timestamp, get_caller_address};
use crate::ticket_booth::structs::{GameExpiration, GoldenPass, GoldenPassInfo, PaymentType};
use crate::ticket_booth::structs::{GoldenPass, PaymentType};
use crate::ticket_booth::ticket_booth;

#[starknet::interface]
Expand Down
2 changes: 1 addition & 1 deletion packages/utilities/src/math/tests.cairo
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
use crate::math::lut;
use crate::math::types::{Fixed, FixedTrait, ONE};
use crate::math::types::{FixedTrait, ONE};

// ============================================================================
// LUT exp2 tests - cover all 33 branches (x=0..32)
Expand Down