Enable "Compiler warnings" preference by default

nuket commented 8 years ago

Describe the request

Enable compiler warnings by default by changing the default value of the "Compiler warnings" preference.

🙂 Important information about potential problems in Arduino code will be visible to more Arduino IDE users. 🙂 Developers of shared Arduino projects will be encouraged to write warning-free code

Describe the current behavior

The default value of the "Compiler warnings" preference is "None"

🙁 Users might suffer from bugs that are difficult to troubleshoot without assistance from the compiler. 🙁 Developers of shared Arduino projects are not aware that their code produces warnings and so don't fix them. These warnings then make it more difficult for users of dependent projects who do enable warnings to differentiate the warnings in their own code from the warnings from the dependencies.

Arduino IDE version

Original report

1.6.6

Last verified with

73b6dc4

Operating system

All

Operating system version

Any

Additional context

Defaulting to not showing warnings is, overall, a bad thing for the ecosystem. Advanced users will have a hard time taking it seriously, when instead of owning the errors, the IDE sweeps them under the rug.

Additional requests

Issue checklist

[X] I searched for previous requests in the issue tracker
[X] I verified the feature was still missing when using the latest nightly build
[X] My request contains all necessary details

ffissore commented 8 years ago

You miss the passage from "totally unable to write a sketch" to "novice". Warnings are unnecessary to the formers and, classes prove, scare them away. Becoming a novice is a big achievement, while going from novice to expert it's just a matter of time and dedication. I'm therefore closing this as wontfix.

nuket commented 8 years ago

Sure, but if Show verbose output during compilation is disabled anyways, then if I've made the effort to enable it, I'd expect warnings to be shown by default. Do novices usually enable that option? Probably not. I would not expect Compiler warnings to be set to None by default. That's useless. As in, what's the point of turning on verbose output to see no additional information that helps you solve your problem?

Setting Show verbose output during compilation to False and Compiler warnings to All. would be ideal.

Again, this should be highlighted in bold on the Arduino website somewhere, because when I see libraries emitting warnings, I know the authors also didn't see them. This isn't just about the novices, this is about the ecosystem of code and advanced users around them too.

ffissore commented 8 years ago

Don't get me wrong: I would turn on warnings by default without giving users an option to turn them off!

However, we have to deal with a ton of already written documentation/books/blogs/tweets/you name it, which (precisely or not) shout out loud what are the side effects of each and every button/checkbox/menu.

This is why we have two distinct options for turning on verbose output and warnings (verbose output is usually used to find out where the IDE has saved intermediate files) And btw, setting warnings level is a recent feature (since 1.6.4)

nuket commented 8 years ago

That would be awesome.

I had used the verbose output to view the commands used, but yes the default assumption that warnings would be provided, that's what caught me off guard. In this case, I think more detail would be better, otherwise, verbose output isn't as helpful as it could be.

I think one underlying reason why warnings are off by default is GCC not having very clear warnings (to novices anyways). Even advanced users usually have to read those warnings twice, just to figure out what the hell GCC is talking about.

ffissore commented 8 years ago

Having clang would really be awesome! We could add autocomplete and use its beautiful error messages. @cmaglie was working on it

nuket commented 8 years ago

Yup. I've actually been working on a script to build Makefiles that would let you use Xcode as an editor and could run syntax checking using the OS X clang. Kind of like X-AVR, but parsing the platform.txt, boards.txt, and programmers.txt files directly to generate projects.

X-AVR already does a pretty good job of getting the indexing and autocomplete working for the native AVR libc and hardware headers.

matthijskooijman commented 8 years ago

@ffissore, I would ask to reconsider this ticket. I asked the original reported to report it after this got mentioned in another issue.

While adding the warning option, we discussed that it would be good to enable warnings by default, at least for the sketch itself (i.e. so only warnings that the user should be able to fix are reported, unlike warnings in the core and library which are effectively opaque to novice users). Also, we considered making a selection of warnings that are clear and easy to fix, and keep the complex warnings disabled by default. We initially didn't implement this, just to get the warning option out there sooner and perhaps collect some feedback, but I still feel that implementing something like this so we can enable (most) warnings by default is important and should be tracked in this issue.

mikaelpatel commented 8 years ago

This has been discussed in detail before. Please see https://github.com/arduino/Arduino/issues/1728.

cousteaulecommandant commented 8 years ago

Although I don't think -Wextra should be enabled by default since as ffissore said it might scare newcomers, I think the -Wall flag would solve a lot of common problems.

-Wall: Are you SURE you meant to write if (x = 0)?
-Wextra: You're comparing a signed with an unsigned, are you CRAZY?!

In other words, -Wall is not really THAT scary; most of the time it indicates actual mistakes. -Wextra is a bit more paranoid and gives many "false positives", so it's probably not a good idea to enable it by default. And in any case, the default warnings (with no -Wall nor -Wextra) are only the REALLY problematic ones, so at the very least I'd remove the -w option.

Also, not using -Wall hides some potential misbehaviors, such as signed overflow:

boolean will_overflow(int x) {
    int y = x+1;
    return y < x;
}

    will_overflow(INT_MAX)  // returns false instead of true

When compiled with -Wall, this shows: sketch.ino: warning: assuming signed overflow does not occur when assuming that (X + c) < X is always false [-Wstrict-overflow], which might be a bit cryptic but suggests that there's probably a problem there. Indeed, regardless of what the Arduino documentation for int says, overflowing a signed integer should never be done, so the compiler is free to assume that'll never happen, resulting in the function being "optimized" so that it always returns false.

matthijskooijman commented 8 years ago

I'm reopening this one, as I still think we should be moving towards having some warnings enabled by default for at least the sketch itself. Also see https://github.com/arduino/ArduinoCore-avr/issues/268, which is somewhat related.

PaulStoffregen commented 8 years ago

I've mentioned this before on the mail list, but not here...

Teensy's platform.txt file enables -Wall for the sketch and all libraries. I also patch Compiler.java to print the only actual errors in red. The compiler's warning output is printed in the same white text as everything else. At least for Teensy, which admittedly has a very different user base than official Arduino boards, this trade-off has worked quite well.

I'd be happy to submit a pull request, if there's a solid decision to go this route.

nuket commented 8 years ago

👍 - I'm in favor of anything that helps people catch latent problems in their code.

oldmanegan commented 8 years ago

I believe that some changes ought to be made in this regard:

Preference by default is verbose mode
The errors are lost in the stream of info that flies through the compiling screen. I recommend the errors are repeated right after or before the final stats (program and variable memory use) so the user sees there are issues

All Library submitters must state that they have run their Libraries in verbose mode and cleared the issues... NB: are even errors reported in verbose mode for basic, widely used Libraries. Fixing these will help newbies (such as me) feel a little more confident - Examples:

/Applications/Arduino.app/Contents/Java/hardware/arduino/avr/variants/standard/pins_arduino.h:74:39: warning: comparison is always true due to limited range of data type [-Wtype-limits]
#define digitalPinToPCICR(p)    (((p) >= 0 && (p) <= 21) ? (&PCICR) : ((uint8_t *)0))

~ and ~

/Applications/Arduino.app/Contents/Java/hardware/arduino/avr/libraries/SoftwareSerial/src/SoftwareSerial.cpp:319:7: note: in expansion of macro 'digitalPinToPCICR'
if (digitalPinToPCICR(_receivePin)) {

PaulStoffregen commented 8 years ago

@matthijskooijman - Could the builder become smarter to try to detect the difference between library authors and ordinary end users? Perhaps if the library source files change more than once (or twice?) after installing or upgrading the library, some sort of state could be saved to remember that particular library is under development? Then the builder could suppress warning for ordinary end users, but show them loudly to the author or any maintainers of that library.... Ideally, this could be done on a per-library (and core lib) basis, so the author of a particular library is only pestered by warnings related to their own code.

matthijskooijman commented 8 years ago

Could the builder become smarter to try to detect the difference between library authors and ordinary end users?

My idea was to (effectively) configure this in the preferences, by letting people (i.e. library authors) enable warnings in libraries, which are disabled by default.

Autodetecting this based on library changes is interesting, but also prone to misfires and unexpected behaviour (user suprise)...

sandeepmistry commented 8 years ago

What about re-using the .development file in the root of the library folder? Currently, if the IDE detects it you can save example sketches for the particular library in place.

matthijskooijman commented 6 years ago

For clarity, let me write down what I had in mind when writing https://github.com/arduino/arduino-ide/issues/1630

Wrt warnings, I would distinguish three areas: Sketches, libraries and cores. By default, warnings would be enabled for sketches, but not for libraries and cores, which facilitates the most common case of a (novice) user writing sketches, but not libraries or cores. For users that write libraries or cores, we can expect some action to enable warnings themselves.

In terms of UI, I can imagine two variants:

Replace the current "warnings level" dropdown by three dropdowns: One for sketches, one for libraries, one for cores.
In addition to the warnings level dropdown, add one "applies to" dropdown, which selects "Sketches", "Sketches and libraries", or "Sketches, libraries and cores".

The former is more flexible (allows different warning levels for different areas, or allows enabling warnings for e.g. cores but not libraries), but might also be slightly more complex for users (OTOH, it does seem easy to understand to me).

This distinction can be implemented in the IDE / arduino-builder, I believe this requires no changes in platform.txt.

The second (and IMHO separate) part of the proposal was to add an extra warnings level "Selected warnings" or "Novice warnings" something like that, in which we enable a selected subset of warnings (rather than relying on some compiler-defined subsets). This subset would be picked by reviewing each individual warning and considering the merits of the warning, as well as the potential to confuse novice users. This subset would probably need to be defined in platform.txt, since it couples tightly with the compiler (version) used.

This proposal does not automatically differentiate between library authors and normal users (as suggested above) but lets users make this choice themselves (defaulting to novice sketch-only users). To add that (either fully automatic or using the .development file sandeep suggests), my proposal could be amended by defining a fourth are "Libraries I'm developing" or something similar, for which warnings are also enabled by default (which also allows disabling them if you are so inclined, which would not be possible if we we would hardcode warnings to enabled for libraries that you're editing).

cousteaulecommandant commented 6 years ago

For an even simpler approach re: the GUI, I'd just add an Include warnings from core/library components checkbox rather than a dropdown menu with 3 options, unless it is really considered necessary to hide core warnings to library developers. Alternatively, just make a series of predefined "profiles", such as "novice", "learning", "developer", "don't bother me", "something is wrong and I have no idea why, please help", etc., each with its own set of flags for each of the three parts. (Whether or not we rely on the compiler's choices is a matter of taste. For example, I would add -Wshadow to the list.) This would limit the users BUT simplify the GUI for those who don't care about such a high level of configurability and just want an easy to use thing.

Since some users may not be OK with such a limited subset, there could be an "Advanced..." button or "Custom" option that opens a dialog where the user can enter their own flags manually (either by typing them or by selecting them from a list), for each of the three parts. I think this would be the best approach, since it provides high customization without interfering with the GUI simplicity.

Finally, I would like to suggest doing some sort of processing of the warning types (as indicated by the [-Wtype-of-warning] in the message), highlighting them in different colors depending on their importance (orange, yellow, white, gray), and possibly adding some aids such as in-editor highlighting and a tooltip explaining what the warning usually means.

brianrho commented 10 months ago

This has been open for nearly 9 years now, and I raised a new issue #2316 which just got closed as a duplicate. Is there a reason it hasn't been fixed yet?

It seems like a fairly simple change to make, with no downsides and clear benefit for the platform.

https://github.com/arduino/arduino-ide/blob/73b6dc4774297e53f7ea0affdbc3f7e963b8e980/arduino-ide-extension/src/browser/arduino-preferences.ts#L113

To be clear, the request is to change this setting to "Default" generally, whether for cores, sketches, etc. Whether people choose to ignore the warnings or manually bump the level up to "-Wall"/"-Werror" should be entirely up to them, but they should have a chance to see these basic warnings first and potentially fix their mistakes.

The default set of GCC warnings should suffice for most usecases (which is why they are default, in the first place), an okay balance for the platform. I don't see any reason why warnings would be any more scary than errors, which users already see and have to fix, so this shouldn't be an argument.

At worst, a user sees some warnings from some library (which compiles just fine) and if they're at all worried, they search or raise a question on some Q/A site where people explain whether the warning is harmless or not -- and/or better yet, they'll raise an issue against the library to find out what's going on, which should in time prompt authors to fix these things or find other ways to do stuff that don't provoke warnings.

This is certainly better than the silent alternative where you spend ages trying to figure out why your device is crashing, when a simple warning would've told you or provided hints immediately.

cousteaulecommandant commented 9 months ago

I think this was never fixed due to lack of consensus. Some views were that defaulting to -Wall shouldn't be done because it would trigger library warnings. Others suggested that maybe libraries should be compiled without warnings and sketches with them, but that implies changing the way the Arduino toolchain works, and that's a lot of work so this caused loss of interest in the idea. My opinion was that libraries shouldn't trigger warnings if they are properly written (they're supposed to be quality code that will stay around forever, not some quick sketch written by an inexperienced user; throwing warnings can be seen as not passing a minimum quality requirement). And finally some think that any warning at all will put off novice users, and it's better to just let them figure out the mistakes the hard way.

Overall, I think that at the very least Arduino shouldn't purposefully disable "natural" warnings, and if a user triggers such a warning it probably means they did something catastrophic and should know about it - after all, Arduino is meant for learning. (I'd personally push for -Wall to be the default for the same reason - these warnings can be didactic and help learn programming - but if it helps getting this issue resolved, I'd be OK with Default being the default as a compromise.)

brianrho commented 9 months ago

Yeah, there's no consensus on -Wall, I don't see enough reason to impose it on everyone, if even professional environments often don't use it.

The default set is already a good default, whether for sketches, libraries, etc., this doesn't need to get terribly complex. I hope it can get resolved soon and doesn't hang around for another decade.

arduino / arduino-ide