Not sure what's the best fix here, just picked one.

I have a mild preference for null instead of a special string

I did think of null, but I have the prediction that whoever calls this will immediately do something like:

let pos = builtins.unsafeGetAttrPos foo bar; in "${pos.file}:${toString pos.line}:${toString pos.column}"

which will then blow up on null.

It should IMO 😀

Less facetiously: What is the "right" behavior for that code in the noPos case?

Well I don't think "(unknown file):0:0" is too bad of an output, but I guess it could be beautified to handle noPos like this:

let
    pos = builtins.unsafeGetAttrPos foo bar;
in if pos.line == 0
     then "(undefined position)"
     else "${pos.file}:${toString pos.line}:${toString pos.column}"

If you're doing that check then why not just check for null (or attr non-existence?)

Oh I had somehow misinterpreted that question as returning { column = 0; file = null; line = 0; } instead of returning null.

IMHO the problem of null there is that now null can mean both "the attribute didn't exist" and "position of that attribute is not known" while it previously could only be the former. So before this change, one could reasonably write code like:

{ name, version, sha256 } @ args:
let
    pos = builtins.unsafeGetAttrPos args "name";
in mkDerivation (args // ...)

with the expectation that pos is always non-null.

unsafeGetAttrPos should probably just throw an exception if the position is undefined.

Throwing an exception sounds pretty inconsistent given that the function already returns null in one error case.

I guess returning null in the crashing case is mostly fine given that nothing in nixpkgs would break on a sudden null. I've updated the commit.