Skip to content

Elaborator: mark_type_as_used and the visibility lint don't recurse through all Type variants #13360

Description

@AztecBot

Split out from #12996 (point 24) as a diagnostic-completeness change that needs a maintainer decision on whether to accept the new warnings/errors it produces.

What a change here would fix

Two exhaustive match typ { ... } walkers omit some nested-type-bearing variants in their trailing => () arm, so a type reachable only through those forms is missed:

  • mark_type_as_used_helper (compiler/noirc_frontend/src/elaborator/mod.rs) recurses Array/Vector/Tuple/DataType/Alias/CheckedCast/Reference/InfixExpr, but no-ops on Function (args/return/env), FmtString (captured tuple), and TraitAsType (generics). It marks structs as constructed to suppress unused/never-constructed warnings, so a struct reachable only via, e.g., a function-typed field (fn() -> Inner) can get a spurious "unused"/"never constructed" warning.
  • check_type_is_not_more_private_then_item (compiler/noirc_frontend/src/elaborator/visibility.rs) recurses DataType/Tuple/Alias/CheckedCast/Function/Reference/Array/Vector/InfixExpr, but no-ops on FmtString captured types and TraitAsType generics. So pub fn f() -> fmtstr<N, (PrivateStruct,)> or a public signature over impl Trait<PrivateStruct> misses the "type is more private than item" diagnostic.

Note the asymmetry: the visibility lint handles Function; mark_type_as_used does not.

What I found that means this needs input

These omissions are reachable (function-typed struct fields; fmtstr/impl Trait in public signatures), but the fix is a diagnostic-completeness change, not a soundness fix — nothing miscompiles today. Adding the missing recursion would:

  • newly suppress currently-emitted "unused struct" warnings (mark_type_as_used) — snapshot/warning tests shift; and
  • newly emit TypeIsMorePrivateThenItem errors on code that compiles today (visibility) — a minor breaking diagnostic change.

Information needed to decide

  1. Do you want these diagnostics to be complete (accepting new warnings/errors on existing code), or is the current partial coverage acceptable / intentional to avoid churn?
  2. For the visibility side specifically: is shipping new hard errors acceptable as a breaking diagnostic change, or should it be staged (e.g. warn/allow-by-default first)?
  3. Should the two walkers be made symmetric (both recurse through Function/FmtString/TraitAsType)?

If you want the change, I can implement it with regression tests (a function-typed struct field for the marker; a public fn returning fmtstr/impl Trait over a private struct for the lint) and reconcile the affected snapshots.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions