Warning for unused function args

[TurkeyMan]

This has annoyed me for years, but recently, multiple people on my team have been angry about it.
They're not D people, and they have had bugs where this would have saved them more time than it took me to work out how to implement it!

The natural resolution to the warning is:

1. Just don't name unreferenced args; ie, arguments to a callback/handler function that's unused by the implementation
2. cast(void)arg; naturally silences the warning.

This is introduced behind -preview=warnunusedparams, which can give people a year or 2 to implement; but we should announce the intention to make it standard at some time in the future, and recommend people enable it and migrate their local code during the lead-up.

[dlang-bot]

Thanks for your pull request and interest in making D better, @TurkeyMan! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please verify that your PR follows this checklist:

• My PR is fully covered with tests (you can see the coverage diff by visiting the details link of the codecov check)
• My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
• I have provided a detailed rationale explaining my changes
• New or modified functions have Ddoc comments (with Params: and Returns:)

Please see CONTRIBUTING.md for more information.

---

If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment.

Bugzilla references

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub run digger -- build "master + dmd#22695"

[thewilsonator]

Warning are generally frowned upon, if you want to get this in, in a reasonable timeframe, it will need to be behind a flag. See also the large number of failures this causes in druntime, not to mention user code e.g. buildkite.

Also this does not play well with static if

void __doPostblit(T)(T[] arr)
{
    // infer static postblit type, run postblit if any
    static if (__traits(hasPostblit, T))
    {
        static if (__traits(isStaticArray, T) && is(T : E[], E))
            __doPostblit(cast(E[]) arr);
        else
        {
            import core.internal.traits : Unqual;
            foreach (ref elem; (() @trusted => cast(Unqual!T[]) arr)())
                elem.__xpostblit();
        }
    }
}

this could be rewritten to

void __doPostblit(T)(T[] arr) if (__traits(hasPostblit, T))
{
    // infer static postblit type, run postblit if any
    static if (__traits(hasPostblit, T))
    {
        static if (__traits(isStaticArray, T) && is(T : E[], E))
            __doPostblit(cast(E[]) arr);
        else
        {
            import core.internal.traits : Unqual;
            foreach (ref elem; (() @trusted => cast(Unqual!T[]) arr)())
                elem.__xpostblit();
        }
    }
+    else 
+         cast(void) arr;
}

to pass with this, but this cannot be merged without a passing CI, and cannot break user code. Are your team using d-scanner? I'm sure this is already a lint rule available there.

[thewilsonator]

And any breakage it would cause would have to be rolled into an appropriate edition.

[TurkeyMan]

Warning are generally frowned upon,

Yeah I know... but sometimes people are wrong ;)

if you want to get this in, in a reasonable timeframe, it will need to be behind a flag.

What kind of flag controls particular warnings like this? I was thinking if it should be -w(/i)=unused_args or something, I'd be happy with that, but it creates a precedent. Maybe that's the way to introduce it and give time for corrections to be distributed though?

I'll rework it in those terms then?

See also the large number of failures this causes in druntime, not to mention user code e.g. buildkite.

Yup, it's a worry! That said, I've scanned through many of the cases, and some of them look like might actually be legit bugs being exposed... can't be sure without investigating them all, but I actually think it clearly demonstrates how much of a problem this is.
I've had numerous bugs where this would have saved time and money. Every codebase I've ever worked in has this option explicitly enabled, for good reason. It's distinctly felt when it's missing. If you never had it, you don't miss it, but when you've used it forever it's a sorely missing tool.

Also this does not play well with static if

void __doPostblit(T)(T[] arr)

Interaction with conditional compilation is well understood by other languages with warn-unused-args; the standard resolution in that case is something to the effect of cast(void)arg; (or in C, you can just do arg; as a no-op statement) to silence in cases where it's not sure if used or not. I actually thought I'd need to make that work in this patch and I imagined that would be 90% of the patch(!), but it turns out that's already implemented and works perfectly to resolve that issue in the 'universally accepted' way.

I'm amazed how tiny this patch is.

[TurkeyMan]

I put it behind a -preview, then we can give it some years for migration, and recommend people enable it to reach compliance in the meantime... probably the best way?

[TurkeyMan]

Are your team using d-scanner? I'm sure this is already a lint rule available there.

No, d-scanner is not integrated into VS. Depending on linting tools like that stratifies the ecosystem, it's the wrong place. VS uses the dmd-as-lib tooling.

[TurkeyMan]

Can you see why those couple failed? I'm not close with the CI ecosystem, so I'm not sure what to expect, but it's not jumping out at me what's wrong with those.

[thewilsonator]

>>> TARGET FAILED: compilable/previewhelp.d

   =fixImmutableConv disallow `void[]` data from holding immutable data (https://dlang.org/changelog/2.101.0.html#dmd.fix-immutable-conv, https://issues.dlang.org/show_bug.cgi?id=17148)
   =systemVariables  disable access to variables marked '@system' from @safe code (https://dlang.org/spec/attribute.html#system-variables)
   =fastdfa          Fast dataflow analysis engine, experimental
+  =warnunusedparams enable warnings for unused function parameters

need to update compilable/previewhelp.d with the above diff

[TurkeyMan]

I'm adding the spec and changelog now... thanks for flagging those other touch-points.
Spec: dlang/dlang.org#4413

[TurkeyMan]

I don't understand that circle.ci failure; and I'm not on a platform where I can follow the instruction in the log output... :/

[thewilsonator]

The Azure ones are spurious, the CircleCI one ons is not. The circleCI has a diff that you need to apply for frontend.h

[TurkeyMan]

The Azure ones are spurious, the CircleCI one ons is not. The circleCI has a diff that you need to apply for frontend.h

Yeah I looked at some past diffs and spotted what I missed (that giant long line!)

[tgehr]

I put it behind a -preview, then we can give it some years for migration, and recommend people enable it to reach compliance in the meantime... probably the best way?

I won't comply and I will fight against making this the default. After many years we finally removed dead code warnings. This is almost as annoying, particularly when conditional compilation is involved.

[0xEAB]

FYI D-Scanner supports checks for both…

• Unused variables.
• Unused parameters (check is skipped if function is marked "override").

[0xEAB]

While I'm all for adding a linter to the compiler itself, I believe this should be done thoroughly and properly — not piecemeal.

I see no value in adding “random” warnings and would prefer an actual linter.

[gorsing]

Go and Rust ship these checks in the compiler for a reason — it works better for the whole ecosystem, not just for those who set up linters.

[gorsing]

Speaking of best practices — in Go and Rust, this kind of check (unused parameters/variables) is built directly into the compiler, not relegated to an external linter. This ensures every developer gets the same feedback regardless of their IDE or CI setup.

Relying solely on D-Scanner creates a fragmented ecosystem where only those who configure it benefit from these checks. Newcomers or teams without strict linting setups miss out on catching potential bugs early.

If D wants to lower the barrier to entry and improve code quality out-of-the-box, following the approach of these languages makes sense. Having this in the compiler (even behind a flag initially) is a step in the right direction.

[rikkimax]

If we are doing this, let's give it its own pass over the modules.

With a dedicated switch like -lint.

[tgehr]

If D wants to lower the barrier to entry

New programmers are usually recommended to get started with languages such as Python, which will let you do type errors at runtime. Breaking their builds will actually throw them off.

and improve code quality out-of-the-box,

It is fundamentally untrue that deleting descriptive parameter names or sprinkling a bit of cast(void)arg over your code base will improve code quality.

following the approach of these languages makes sense.

This PR does not even do that. Also, Go and Rust don't have D's approach to conditional compilation, nor to warnings, so you can't simply copy-paste anyway.

[gorsing]

"New programmers are usually recommended to get started with languages such as Python, which will let you do type errors at runtime. Breaking their builds will actually throw them off."

We're not talking about runtime type errors — we're talking about compile-time warnings for unused parameters. These are two completely different things. Python's approach is irrelevant here because Python doesn't have a compile stage at all.

Also, nobody is proposing to "break builds" by default. This PR puts it behind -preview flag, exactly so that nobody gets surprised. The whole point is to give the ecosystem time to migrate.

"It is fundamentally untrue that deleting descriptive parameter names or sprinkling a bit of cast(void)arg over your code base will improve code quality."

That's a strawman. The improvement comes from detecting unintentionally unused parameters. The two resolutions you mentioned are exactly that — resolutions for when the parameter is intentionally unused. They're not the feature, they're the escape hatch.

The actual quality improvement happens when a developer accidentally leaves a parameter unused and the compiler says: "Hey, you declared ctx but never used it — did you forget something?" That's caught real bugs for me and my team, multiple times.

"This PR does not even do that. Also, Go and Rust don't have D's approach to conditional compilation, nor to warnings, so you can't simply copy-paste anyway."

Of course it's not a copy-paste — nobody is asking to copy Go's or Rust's code. The point is about philosophy: these languages treat basic code quality checks as a compiler responsibility, not an optional external tool.

[tgehr]

Disgusting AI slop.

[gorsing]

@tgehr, I respect your expertise and deep history with D's semantics. Fair enough, let's put the phrasing aside. My only goal is to discuss how we can improve the D ecosystem's baseline quality.

The -preview flag is exactly for what you mentioned: finding where it doesn't play well with templates or static if and refining it before anything becomes a default. I'd appreciate your technical input on those edge cases so we can make the check smarter, rather than just relying on external tools.

[rikkimax]

The -preview flag is exactly for what you mentioned: finding where it doesn't play well with templates or static if and refining it before anything becomes a default.

This will never be the default.

Walter is very against warnings. Either they should be errors, or they shouldn't exist.

This is why the fast DFA engine can only produce errors, things it knows 100% are incorrect behaviour.

[tgehr]

The only way refusing compilation for a linter check that is rather unspecific might ever be a valid compromise for anyone is if you are technically unable to do that check in the appropriate way, at the appropriate place, with the appropriate severity.
Furthermore, introducing unnecessary friction also drives bugs because it wastes competent people's time on trivial little structural things that are not all that impactful overall. Why you would ever want to break your own build because some perfectly valid and thoroughly tested third-party code you are importing decided e.g. to consistently name the parameters on methods in a class hierarchy or used a static if to choose which parameter to use is just beyond me. It makes no sense. Run the linter on your own code, maybe use CI.

I mean... what you're illustrating here is why I fundamentally disagree with Walter about warnings in general. I value the agency to enable and disable those that apply to my project/workplace/team in a deliberate and intentional way.
...

Here are some necessary preconditions for me not objecting to this kind of thing:

• Whether a build works or fails should never depend on the optional linter check passing or failing.
• The runtime semantics of a valid piece of D code should never depend on the optional linter check passing or failing somewhere.

I.e., if you actually want this to break builds, it has to be enabled in the source file, not on the command line.

[rikkimax]

Here are some necessary preconditions for me not objecting to this kind of thing:

• Whether a build works or fails should never depend on the optional linter check passing or failing.
• The runtime semantics of a valid piece of D code should never depend on the optional linter check passing or failing somewhere.

I.e., if you actually want this to break builds, it has to be enabled in the source file, not on the command line.

pragma(lint);
pragma(lint, lintParams);

Where lintParams is of type LintParams struct, that configures it.

Remove -w and -wi.

I'm ok with this.

[tgehr]

Here are some necessary preconditions for me not objecting to this kind of thing:

• Whether a build works or fails should never depend on the optional linter check passing or failing.
• The runtime semantics of a valid piece of D code should never depend on the optional linter check passing or failing somewhere.

I.e., if you actually want this to break builds, it has to be enabled in the source file, not on the command line.

pragma(lint);
pragma(lint, lintParams);

Where lintParams is of type LintParams struct, that configures it.

Remove -w and -wi.

I'm ok with this.

Me too.

[tgehr]

In any case, I think making people either use the parameter or delete the parameter name completely is bad UX. Other languages don't do it like this.

[TurkeyMan]

In any case, I think making people either use the parameter or delete the parameter name completely is bad UX. Other languages don't do it like this.

Deleting the name is not universal; stating a dummy reference is more typical. Both options are valid in D; that's not by design here, that's just how the language already happens to be.

Your point about a trivial lambda is a thing; this must be excluded from lambdas.

Here are some necessary preconditions for me not objecting to this kind of thing:

• Whether a build works or fails should never depend on the optional linter check passing or failing.
• The runtime semantics of a valid piece of D code should never depend on the optional linter check passing or failing somewhere.

Good news; -wi is for you! :P

[TurkeyMan]

Here are some necessary preconditions for me not objecting to this kind of thing:

• Whether a build works or fails should never depend on the optional linter check passing or failing.
• The runtime semantics of a valid piece of D code should never depend on the optional linter check passing or failing somewhere.

I.e., if you actually want this to break builds, it has to be enabled in the source file, not on the command line.

pragma(lint);
pragma(lint, lintParams);

Where lintParams is of type LintParams struct, that configures it.

Remove -w and -wi.

I'm ok with this.

Ewoo. We already have perfectly good and universally conventional mechanism; why would we invent new things that are just weird and/or worse than the things we already have?
D has -w and -wi for decades; if you don't like it, don't write it. I like it, I write it, but it's distinctly lame that we can't be selective; all-or-nothing prescription and the consequence that it's impossible to introduce new ones just leads to a shit-fight like this.

In my last ~30 years in the trenches, there have been an uncountable number of new warnings added to C/C++, and every project I've ever worked on has always enabled warnings-as-errors. It's not a matter for debate that teams of hundreds (or thousands at Blizzard) of engineers overwhelming prefer and invite compiler warnings, and happily accept new ones as they appear all the time.

The number of times that the introduction of new warnings has actually annoyed people or caused material issues; I could count it on one hand, maybe once-or-twice a decade. In a few rare cases, a project has had to add -w=disable:whatever because it was difficult to comply, or particularly socially or politically unpopular for some reason... and that's the end of the story! disable it, move on, no problem here! In the overwhelming case, people invite those warnings; in general, they tend to represent lessons learned over time, and they guide a community towards better uniformity without making it draconian. I like warnings as a mechanism, and I'm happy for them to exist; we should make BETTER use of the mechanism.

And they do exist in D, they always have; that doesn't seem like a matter for debate. If we could selectively enable/disable warnings, we'd be in the same (perfectly workable) situation that the rest of us have enjoyed for decades.

🤷‍♂️

[gorsing]

​@0xEAB I completely agree that D-Scanner is a fantastic tool for the ecosystem. However, there is a fundamental technical divide here: D-Scanner operates primarily on the AST, whereas this check benefits immensely from full semantic analysis.

​Only the compiler has the final semantic graph—it’s the only tool that truly "understands" which branch of a static if is taken or what a mixin actually generates. This isn't about duplicating a linter; it's about using the compiler’s unique position to implement the intelligent heuristics suggested by @ntrel. This level of precision is exactly what's needed to avoid the false positives that often plague external tools in complex D code.

[tgehr]

Here are some necessary preconditions for me not objecting to this kind of thing:

• Whether a build works or fails should never depend on the optional linter check passing or failing.
• The runtime semantics of a valid piece of D code should never depend on the optional linter check passing or failing somewhere.

Good news; -wi is for you! :P

Nope, I don't want these warnings at all. However, dub puts -w by default, so if someone else will import my code, it will likely break their build. I think you don't understand what "never" means.

[tgehr]

We already have perfectly good and universally conventional mechanism

Here's where you are wrong. It's a very bad mechanism.

[tgehr]

$ dmd/compiler/src/dmd$ grep 'warning(' *.d
cparse.d:                eSink.warning(startloc, "current pack attribute is default");
cparse.d:                eSink.warning(startloc, "current pack attribute is %d", packalign.get());
doc.d:                                sc.eSink.warning(s.loc, "Ddoc: function declaration has no parameter '%.*s'", cast(int)namelen, namestart);
doc.d:                sc.eSink.warning(s.loc, "Ddoc: parameter count mismatch, expected %llu, got %llu",
doc.d:                    eSink.warning(loc, "Ddoc: Stray ')'. This may cause incorrect Ddoc output. Use $(RPAREN) instead for unpaired right parentheses.");
doc.d:                    eSink.warning(loc, "Ddoc: Stray '('. This may cause incorrect Ddoc output. Use $(LPAREN) instead for unpaired left parentheses.");
dtoh.d:            warning(loc, "%s `%s` is a %s", kind, ident.toChars(), reason);
errors.d:    void vwarning(Loc loc, const(char)* format, va_list ap)
errors.d:    extern (C++) void warning(Loc loc, const(char)* format, ...)
errors.d:    pragma(printf) extern (C++) void warning(Loc loc, const(char)* format, ...)
errorsink.d:    void vwarning(Loc loc, const(char)* format, va_list ap);
errorsink.d:    void warning(Loc loc, const(char)* format, ...)
errorsink.d:        vwarning(loc, format, ap);
errorsink.d:    void vwarning(Loc loc, const(char)* format, va_list ap) { }
errorsink.d:    void vwarning(Loc loc, const(char)* format, va_list ap)
expressionsem.d:            warning(loc, "`%s %s %s` is performing truncating conversion", type.toChars(), EXPtoString(op).ptr, t2.toChars());
frontend.d:            return warning(loc, format, ap, p1, p2);
frontend.d:    abstract bool warning(const ref SourceLoc loc, const(char)* format, va_list args, const(char)* p1, const(char)* p2);
frontend.d:    override bool warning(const ref SourceLoc loc, const(char)* format, va_list args, const(char)* p1, const(char)* p2)
inline.d:        warning(fd.loc, "cannot inline function `%s`", fd.toPrettyChars());
lexer.d:                                        warning(t.loc, "char 0x%x is not allowed start character for an identifier", tempDchar);
lexer.d:                                        warning(t.loc, "char 0x%x is not allowed continue character for an identifier", tempDchar);
lexer.d:    void warning(T...)(Loc loc, const(char)* format, T args)
lexer.d:        eSink.warning(loc, format, args);
parse.d:            eSink.warning(elseloc, "else is dangling, add { } after condition at %s", lookingForElse.toChars());

I.e., there is very little precedent and all of these should just be errors (except maybe the one in the C parser, idk). Your new one should not be an error.

[limepoutine]

The inliner one should really be removed (I know, that the entire frontend inliner should be, but doing so would result in surprising performance regression). It's the weirdest errorwarning message in DMD.

[ibuclaw]

#21780

And some way towards a natural conclusion of "warnings".

ibuclaw/dmd@73e46b5

[0xEAB]

​@0xEAB I completely agree that D-Scanner is a fantastic tool for the ecosystem. However, there is a fundamental technical divide here: D-Scanner operates primarily on the AST, whereas this check benefits immensely from full semantic analysis.

@gorsing I completely agree that LLM is a fantastic tool for writing messages. However, there is a fundamental technical divide here: If you’re trying to disagree with me, do not copy my own arguments because that makes it look like you kinda agree with me.

[TurkeyMan]

We already have perfectly good and universally conventional mechanism

Here's where you are wrong. It's a very bad mechanism.

https://www.youtube.com/watch?v=j95kNwZw8YY

[WalterBright]

@tgehr > Walter is fundamentally against fine-grained flags to enable and disable warnings, and he has spoken out against having any warnings in the first place.

The rationale is every warning bifurcates the language into two languages. 3 warnings mean there are 8 D languages. Having 2^n languages trying to coexist is a big problem. C & C++ have this problem, as they went wild with warning messages, often resulting in source code that will compile with one compiler but not with another and vice versa (with all warnings turned on, as different compilers have different notions of what should be warned).

The existence of warnings is also a sign of an inability to make a decision about how the language should behave.

BTW, every switch the compiler command line has is a bug. Unfortunately, I don't know how to do without them.

[tgehr]

We already have perfectly good and universally conventional mechanism

Here's where you are wrong. It's a very bad mechanism.

https://www.youtube.com/watch?v=j95kNwZw8YY

https://youtube.com/watch?v=cLJQgEJ9sps&t=875

[WalterBright]

Food for thought - warnings are currently turned on globally with -w. Instead of globally, warnings can be enabled only at the module level. I.e. add to the module declaration the ability to turn on warnings for that module, and that module only. A LintAttribute could be added to the ModuleDeclaration. Something like:

@lint module foo;

This would enable compatibility between modules with linting turned on or off.

[rikkimax]

Food for thought - warnings are currently turned on globally with -w. Instead of globally, warnings can be enabled only at the module level. I.e. add to the module declaration the ability to turn on warnings for that module, and that module only. A LintAttribute could be added to the ModuleDeclaration. Something like:

@lint module foo;

This would enable compatibility between modules with linting turned on or off.

#22695 (comment)

Not as big of a change, and allows for configurability.

[WalterBright]

If we could selectively enable/disable warnings, we'd be in the same (perfectly workable) situation that the rest of us have enjoyed for decades.

I've done the enable/disable particular warnings thing back in my C days. After a while, it just gets to be a nuisance. Turn them all on, or turn them all off. Bing bang done. Warnings do change over time, and our marvy new "editions" feature should take care of that problem.

[Herringway]

I think using a linter to do linting is an option to be considered.

[WalterBright]

A separate linter is still an option, as it can be much more aggressive than what you'd want in the compiler.

[Herringway]

A separate linter is still an option, as it can be much more aggressive than what you'd want in the compiler.

Is there something to be gained from duplicating the functionality, aside from even more code to bitrot?

[rikkimax]

A separate linter is still an option, as it can be much more aggressive than what you'd want in the compiler.

Not really, a linter in the compiler can be just aggressive if you want it to be.

module common;
enum LP = LintParams(agressive: true);

module something;
import common;

pragma(lint, LP);

[WalterBright]

I don't anticipate the necessity of duplicating the functionality. A separate linter would check for things the @lint doesn't.

[WalterBright]

a linter in the compiler can be just aggressive if you want it to be

A separate linter can afford to make expensive (compute time and memory) checks.

[rikkimax]

a linter in the compiler can be just aggressive if you want it to be

A separate linter can afford to make expensive (compute time and memory) checks.

Ah I see, you're conflating static analysis for linting.

No, the linting being wanted here is all really simple syntax-level stuff.
Basically what dscanner does.

Anything else would likely need to be requested specially if it were to ever exist (doubtful).

[TurkeyMan]

Ah I see, you're conflating static analysis for linting.

"linting" is a synonym for "warning" used by under-30's, because everything needs new terminology to remain trendy ;)

[rikkimax]

Ah I see, you're conflating static analysis for linting.

"linting" is a synonym for "warning" used by under-30's, because everything needs new terminology to remain trendy ;)

No, it's not, it's a type of analysis that can produce warnings.

https://softwareengineering.stackexchange.com/questions/367848/difference-between-linter-sanitizer-and-static-analysis-tools

Warnings are a log level, which is different from an analysis strategy.

The reason I had to check to see if Walter was conflating static analysis and linting, is that static analysis is implemented with a DFA, like the fast DFA engine, only Walter jumps straight to chaotic iteration, which is expensive, so he can be against something that isn't actually what was being requested.

In this case what is being requested is linting capabilities like dscanner, but in the compiler instead of split out.

[TurkeyMan]

In this case what is being requested is linting capabilities like dscanner...

I'm pretty certain that what was requested is quite explicitly, a WARNING, with conventional warning semantics. I can assure you, I've never asked for a 'linter'...

If you wanna split hairs; the features of a typical 'linter' which I reckon you might reasonably separate from warnings, would be trivia like style-guide violations; that's obviously not a warning.
I'll grant that distinct terminology like "linter" should apply to things like style checkers, but an unused variable is a traditional warning, no matter what program generates that complaint.
Making a warning engine external is pretty dumb, you basically have to compile your program twice; once for the program, another time for the analysis, and the 2 tools could easily (often) have language versions that are out of sync.