rust-lang / rust

Empowering everyone to build reliable and efficient software.
https://www.rust-lang.org
Other
98.55k stars 12.74k forks source link

rustc silently ignores a class of errors in flexible target json files #24384

Closed ghost closed 6 years ago

ghost commented 9 years ago

Currently rustc will allow for extra fields to be added to flexible target json without notifying the user, even with --verbose. For example, the following is accepted:

{
    "THIS-IS-INCORRECT": "but rustc will silently accept it anyway",
    "data-layout": "e-p:32:32-f64:32:64-i64:32:64-f80:32:32-n8:16:32",
    "llvm-target": "i686-unknown-linux-gnu",
    "target-endian": "little",
    "target-pointer-width": "32",
    "arch": "x86",
    "os": "linux",
    "morestack": "false",
    "pre-link-args": "-nostdlib",
    "dynamic_linking": false
}

The last three entries have errors that could be easy to miss. This is worrying, because for all the optional fields, simple errors such as misspellings will cause those fields to be discarded and the default values to be used instead. To the programmer it could seem as if the configuration is in place when in reality the compiler simply went with the default value.

I had one of these happen to me earlier and I almost missed it completely. I think that this should at the very least trigger a warning.

steveklabnik commented 9 years ago

This isn't strictly diagnostics, but close enough.

emberian commented 9 years ago

Yeah, this has annoyed me recently too, when I originally did this I decided not to validate the precense of extra fields for compatability with future fields in other rustc versions (As otherwise it's not possible to have a new target spec work on an old rustc at all). I'm pretty sure that we should NOTABUG that, but do stricter validation for types.

steveklabnik commented 7 years ago

Triage: I don't believe that anything has changed here, but I can't check exactly right now.

Mark-Simulacrum commented 7 years ago

This seems like we'd want to do stricter validation of types. I'm somewhat surprised we don't today, but it's possible this could be fixed by moving to Serde for deserialization (even if just into serde_json::Value) since it does far more strict validations.

steveklabnik commented 6 years ago

So, I thought that there was a PR to fix this, but I cannot find it. Regardless, when trying to build a project using the above target spec:

error: failed to run `rustc` to learn about target-specific information

Caused by:
  process didn't exit successfully: `rustc - --crate-name ___ --print=file-names --target \\?\C:\Users\steve\src\intermezzos\kernel\intermezzos.json -
-crate-type bin --crate-type rlib --crate-type dylib --crate-type cdylib --crate-type staticlib --crate-type proc-macro` (exit code: 1)
--- stderr
error: Error loading target specification: Field target-c-int-width in target specification is required
  |
  = help: Use `--print target-list` for a list of built-in targets

fixing some of it up:

error: Error loading target specification: pre-link-args: expected a JSON object with fields per linker-flavor.

so, it does seem like this has been fixed. Closing!