I understand the patch and I think it makes sense.

Path.find_free_opt ids p checks if one of the identifiers in ids occurs in p, and the ids of the nondep_* functions are the "forbidden" identifiers that one is not allowed to depend on.

Currently the code in the Tconstr(p, tl) tries to expand the Tconstr away if p contains a forbidden identifier, and otherwise just maps itself recursively on the tl. But sometimes one of the tl will fail, that could have been erased by expanding a non-dependent p; the PR tries to expand p in this case (it does not use a forbidden identifier, but one of its parameter does). I think that a property we get is that whenever there is a failure on a Tconstr, we always tried expanding before failing.

Question: two other cases in this function (Tpackage and Tobject) also use Path.find_free_opt p on a path. Would it also make sense to try to expand there? (Does expansion make sense for class types? I don't think it does for package paths -- which are already normalized as paths.)

I think the code could be rewritten to look as follows:

try
  Option.iter
    (fun id -> raise (Nondep_cannot_erase id))
    (Path.find_free_opt ids p);
  Tconstr(p, List.map ...)
with Nondep_cannot_erase _ as e ->
  match nondep_expansion () with
  | Some res -> res
  | None -> raise e

Question: two other cases in this function (Tpackage and Tobject) also use Path.find_free_opt p on a path. Would it also make sense to try to expand there? (Does expansion make sense for class types? I don't think it does for package paths -- which are already normalized as paths.)

No, expansion only has any effect for Tconstr (see expand_abbrev_gen above)

I think the code could be rewritten to look as follows:

I found the Option.iter ( ... raise ... ) logic harder to follow than an explicit match, but other than that I like this approach. I've changed the implementation.

This makes sense to me too.
Just to be sure, since this is old code, does @xavierleroy have an opinion on this ?

This is old code indeed. From memory:

• Expanding type abbreviations to try to get rid of a dependency is sound, as far as I can see.
• Back in the days the party line (set by @diremy and @garrigue) was to never expand abbreviations except when absolutely forced to during unification, because the abbreviation usually stands for a humongous recursive object type or polymorphic variant that will make programmers miserable when displayed in an error message. I'm afraid the same problem can occur here.

Indeed; we would argue that we are "forced" to expand the abbreviation here, as otherwise the equation gets erased, we strictly lose information, and typing may fail later down the road.

I agree with @gasche: this patch expands only in cases where nondep would otherwise fail. @xavierleroy does that answer your concern?

I am considering merging this if we don't have further feedback in the next few days. I think Xavier's feedback was more of an encouragement for other people to voice potential concerns, so we should be fine if no concerns are voiced.

Yes, I'm reassured. Users will still not like the weird recursive types they get after expansion of a big object type or variant type, but that's better than failing.