Skip to content

Cursed coercions in FromValue #14147

@sholderbach

Description

@sholderbach

There are a few type coercions in our current provided implementations of FromValue that may be unexpected (and also break bijectivity with IntoValue).

I see this as problematic as several commands use CallExt's methods to gather arguments relying on FromValue to perform the runtime type check.

e.g.

let my_int: i64 = call.req(0)?;
  • impl FromValue for i64
    • on top of Value::Int will also convert Duration and Filesize (they currently store their inner value with i64)
    • sleep's implementation currently uses this behavior to ingest durations.
    • spotted this while trying to resolve the test failures in Replace IncorrectValue with InvalidValue #14079 here a run time type check was simplified to the CallExt variant for i64 expecting ints and tests for window/chunks failed due to silent type coercion.
  • impl FromValue for NuGlob
    • Value::Glob and Value::String make sense if you have explicit glob semantics.
    • Unsure about Value::Cellpath, we want to change its string repr with Cell path display output doesn't escape/quote properly #13362 etc. so that will break (need to trace if there is a command/op that has a signature crime in that direction)
  • Some minor soft coercions that may be surprising
    • impl FromValue for Vec<u8>
      • also coerces Value::String

Are there outside dependents on this coercing behavior in derive use of FromValue @cptpiepmatz ?

Otherwise I am inclined to restrict this to the more restrictive corresponding types and add additional typed impl FromValues as necessary. This would make the use of CallExt or similar conversions more predictable. Alternatively we would have to actively discourage its liberal use and use another mechanism for runtime type checks.

Metadata

Metadata

Assignees

No one assigned

    Labels

    A:type-systemProblems or features related to nushell's type system

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions