Clang doesn't report unused parameters

[loherangrin]

Describe the bug Clang doesn't report warnings about unused parameters, whereas MSVC is considering them as blocking errors. The issue is affecting new projects that are using the engine source code. For instance, the following code block is working on Linux platforms, but it throws warning C4100: unreferenced formal parameter errors on Windows:

class TilesNotification
{
    [...]
    virtual void OnTileEnergyChanged(const AZ::EntityId& i_tileEntityId, float i_normalizedNewEnergy){}
}

The offending code can be easily fixed adding a [[maybe_unused]] to parameters of the previous function. However, as a developer that relies upon Clang only, it could be difficult to spot all these cases when the project becomes bigger, since they have to be reviewed manually.

I was able to force the desired behavior on Linux adding the following parameter to Configurations_clang.cmake file on my system. However, due to impacts that it can have on the default installation, I want a your confirmation about the better solution to use:

#############
# Enabled warnings (that are disabled by default)
#############
-Wunused-parameter

Steps to reproduce

1. Clone v1.0.0 tag of the project: https://github.com/loherangrin/games.o3de.o3de-jam-2305
2. Build on Ubuntu 22.04 with Clang 14.0.0. It doesn't return any error.
3. Build on Windows with MSVC 2022 and review the blocking errors.

Expected behavior Both compilers have to return the same set of blocking errors for generic code that isn't platform-dependent. In order to get a better code quality, it is desirable that Clang reports unused parameters as MSVC already does.

Actual behavior The build is failing on Windows, but it still passing on Linux.

Found in Branch main (2305.0)

Commit ID from o3de/o3de Repository 8c1421e2a0d168d34ae9ca6c763270fe8395a809

Desktop/Device:

• Device: PC
• OS: Windows and Ubuntu

[BytesOfPiDev]

@loherangrin @amzn-changml I enabled -Wunused-parameter and it resulted in a massive amount of unused variables all throughout the engine when building in release and profile. I don't mind fixing them and submitting a PR, but I wanted to confirm if that's okay before going through all of them.

I'd be decorating ones that are sometimes used based on macros with [[maybe_unused]] and /*commenting*/ ones that are never used.

[lemonade-dm]

Turning on this option is tricky if there are already a ton of unused variables throughout the engine.

Fixing the unused parameters within the o3de/o3de repo is a known quantity, but what is unknown is that external gems and projects with source code that has unused parameters, would start to fail to compile if the option was turned on.

The counterpoint to this is that the MSVC compiler warns about unused function parameters However for some reason a lot of unused function parameter get through the MSVC compiler without being flagged. Occasionally a new version of of the MSVC 14.30 toolset (VS2022) causes a warning to trigger that isn't caught.

@BytesOfPiDev Do you know what the ballpark range of how many files trigger the unused parameter warning? If it is somewhere above 20 different files, chances are that source files in o3de/o3de-extras repo, as well as other external gems have at least one function with an unused parameter, that isn't marked [[maybe_unused]] and this may break them.

Personally I would want the compiler option on if we didn't have any concerns with causing several external libraries to fail to compile, but it is highly likely that Clang will catch unused function parameters that the MSVC compiler is not currently catching

[BytesOfPiDev]

@lumberyard-employee-dm

In the 15 minutes I was fixing some, I had ~6000 (no unity) files to compile and the first 300 or so had more than 30 files with used errors, some with multiple (about half of them headers). I'm not sure on the total amount since compilation failed pretty quickly after fixing one file.

I'd want it enabled too, but I've also noticed the inconsistency with MSVC. On the other hand, there's going to be an inconsistency whether or not it is enabled since MSVC doesn't catch them all. At least with it enabled, there'd be less unused variables overall. The external libraries, I assume, are much smaller than the o3de repo and would take relatively little time to fix any unused variables errors in comparison. It could be broadcast as a breaking change with enough warning before it becomes merged.

At the moment, I was wanting to at least fix the ones inside the o3de engine, without committing the -Wunused-parameter option.

[lemonade-dm]

@lumberyard-employee-dm

In the 15 minutes I was fixing some, I had ~6000 (no unity) files to compile and the first 300 or so had more than 30 files with used errors, some with multiple (about half of them headers). I'm not sure on the total amount since compilation failed pretty quickly after fixing one file.

I'd want it enabled too, but I've also noticed the inconsistency with MSVC. On the other hand, there's going to be an inconsistency whether or not it is enabled since MSVC doesn't catch them all. At least with it enabled, there'd be less unused variables overall. The external libraries, I assume, are much smaller than the o3de repo and would take relatively little time to fix any unused variables errors in comparison. It could be broadcast as a breaking change with enough warning before it becomes merged.

At the moment, I was wanting to at least fix the ones inside the o3de engine, without committing the -Wunused-parameter option.

Fixing the unused parameters within the engine is perfectly fine and I would be good with that being done.

Turning on the option is where it gets tricky Forward communication would be needed to let users know that the changes is potentially breaking.

The broadcast can be sent to the external-gems-plugns channel as well as the [impactful-changes] channel in the o3de discord to let users on it know about it beforehand.

Also this may be release note worthy to inform Gem developers that specifies dependencies on a specific engine version that they may have to before code changes if or restrict the valid range of o3de version the Gem can build on.

[loherangrin]

Thanks @BytesOfPiDev for trying to fix this issue.

I agree with @lumberyard-employee-dm about the impacts that this change may have on external systems. For instance, after finding this behavior, I tried to move the option to my other projects (not the one linked above) and the amount of errors that I have to fix on my side (not engine-related) was greater than what I'm expecting. It is obviously my fault that I didn't add [[maybe_unused]] until now, but since at the moment I'm targeting Linux systems only, I didn't have any feedback about when it was required. And other developers might be in the same scenario, especially if they have some public-released gems. Also note that there are other warning categories that are ignored on CLang, but MSVC is still reporting (e.g. some implicit cast).

I'm just thinking if it could be possible to use a softer way to add this feature, so that a new breaking change isn't added to the next release:

1. allow developers to override compile options on a per-project/gem basis, selecting them from a preset list of cross-platform validation levels (e.g. none, weak, strict, all) and/or adding any arbitrary parameter (e.g. custom level). So they can easily opt-in / out from some specific compilation restrictions, and it would eventually support those scenarios where a gem requires some special options that aren't compatible with the main building process of the engine. I believe that the latter can be already supported by the PAL system (ly_append_configurations_options in a gem?), so I would be glad if you can point to any existing documentation / example;
2. add a warning message when a gem doesn't contain any choice about the validation level. This would notify developers that an action is required on their side to keep their projects compatible with future O3DE versions, but it doesn't break them for now. Just like it was done when o3de_pal_dir function was introduced;
3. set all current templates to strict level, so that new gems and projects will be developed with more restricting validation requirements than the current version of the engine, which is kept to the current weak level.
4. in the next release of this year (2310.0), report the new feature in the changelog encouraging developers to update their gems;
5. at some point in a future release (24xx), switch the engine to match templates (i.e. weak > strict) since developers should have already taken an action on their side.

While validation levels are easy to use, I'm not totally sure if it is the same for designing and maintaining them since there is no direct matching between behaviors of different compiler versions (e.g. if a new parameter should be added after point 6 for MSVC 20xx but not for CLang yy, there is the need to create a stricter level, then a more-stricter one, and so on). So maybe access to raw compile parameters could be enough (e.g. -Wunused-parameter, -Wconversion), if we don't want to mix validation levels with the new engine versioning system.

Note that I don't have enough experience on the O3DE build system to achieve all these points in a PR, so this is only a proposal for discussion and eventually give an input to someone to take over it, if it is worth.

[BytesOfPiDev]

@loherangrin

I will have a go at creating an RFC and an example implementation based on what you've mentioned, as well as fixing the errors in the engine.