Currently most people rely on Merlin or RLS for error reporting, however, such tools are not robust to infer all errors.
Only the build system is the reliable source of truth to tell users if everything is okay. However, there’s one in-accurate place with regard to warnings. When the build system do the clean build, it will report warnings faithfully, however, during the second time, since the existing targets are already built, the warnings are completely gone, the build output can not faithfully report those warnings which is a source of frustration.
By treating warn-as-error by default, if there’s warning, it will be error, the build output is always reliable.
Other benefits
Less user-level options, less things to learn
Work around needed
The work around in my head is that user can do file level tweaks to suppress some warnings temporarily, like put @@@warning "a" (or @@warning("a") in ReScript syntax) in the beginning of a file, is such work around enough?
I don’t like when warnings block the build and are treated as errors. Sometimes in a production setting you want to temporarily ignore a certain warning without going in config and ignoring the warning globally. It blocks shipping.
Hi, you can turn it off either project-wise by adding “-w a” to your config (not very recommended) or file-wise with attribute.
There’re trade-offs here, the major benefit is that the build output is a reliable source of truth for error reporting and our editor story will benefit from this.
Previously we use merlin or other tools for error reporting, it’s far from ideal, we came across lots of false alarm or files not opened not reported as error.
Note we’re not alone, go build always treat warnings as errors, we are actually better than go since we leave reasonable work around
Our approach is to have warnings as errors disabled during development, but enabled for CI builds. For this, we patch the bsconfig.json during the CI build process.
The reason is that, during development, I do not care whether I have an unused function parameter or an unused variable somewhere when I want to quickly test the code. However, before a PR is merged, everything should be completely clean.
So what I’d rather like to see is an easy way to have different flags enabled for different build types (dev vs. release).
Well, if you ship code with warnings as a stopgap, maybe you should put a @@warning there, complete with a todo comment and/or issue, so you can fix it later.
I think it depends on your expectations. Coming from ESlint, I sort of expect to see the warnings in the code editor, but not in the build output, because the builds are usually configured to not stop on warnings, so rereading them on every rebuild can be seen as useless noise. Then again, a lot of the ESlint rules are just nitpicking, while the ReScript warnings are usually meant to catch bugs, so they are probably more important, and you should strive to have zero warnings.
Yeah, we do something similar with my current TypeScript/ESLint project: when we develop, we only care if the TypeScript typechecks, because it catches a lot of bugs early. But linting is run on pre-commit and in CI.
Just today I personally had to fix a production bug that would have been stopped by a fatal warning. Once a codebase is littered with warnings, it’s very difficult to clean it up again–there are all sorts of obstacles. Imho it’s better to keep discipline here.
This is my thinking as well. I’m in favour of this change, if we can have a “dev” mode (which has been discussed previously) that reverts back to warnings as warnings.
If you have so many warnings that you can miss new critical ones, having the compiler enforce warning-as-error in production may be a difficult change to adopt
The major motivation is to help editor always report warning/errors reliably.
Other parts are trade offs we have to make, note we don’t close the door like go, you can always disable warnings if you don’t want to get it blocked
Is there not a solution to both have good errors and good warnings?
To me only having warnings correctly reported in a clean build is not a big problem. This solution is worse when it comes to developer experience. It sucks when developing to have to compiler not compile because you are not using a variable right now. So I would encourage to either find a solution that has proper warnings OR if that’s too tricky with the current compiler, it’s not a big problem we are facing at the moment (warnings generally don’t break things).
At Ahrefs, we use "warnings": {"error": "+A-d"}, by default in all the frontend codebase, so what Bob proposes is aligned with our setup. This configuration forces us to go to some extra lengths sometimes to remove warnings from generated code (e.g. https://github.com/ahrefs/atd/pull/214) but we think it’s worth it. In our experience, it is much cheaper to fix a warning in context, while one is editing the file, rather than waiting for CI to report the error way later in time.
It’s great to also leave a work around / escape hatch if needed.
I would be happy to hear if you have a better solution for reliable editor error reporting
@Hongbo if the editor extension somehow called bsc.exe on the currently edited file individually, then warnings would not be lost between runs, right? (I understand this would involve having the editor somehow reading from build.ninja… but just curious if it’d be a viable approach).
Unless I misunderstood–the compiler wouldn’t enforce anything different for existing codebases. We are talking about warning and error settings for new codebases only, i.e. changing the recommended bsb -init templates. But maybe @Hongbo can confirm.
After some hacks, I think we can still generate js files even with non-critical errors, however, we need fake the time stamp of generated js files to notify it’s dirty. (it works on my OS, need test Windows)
In this case, would it be better?
I didn’t have time to respond to the announcement thread yesterday, I can understand why errors are better from a build system perspective but I definitely rely on continuing to generate JS with non-critical errors. I will put up with any hacks required to do so and I support this move.
A future IDE improvement could even flag non-critical errors as warnings in the IDE
