SpacemanDMM icon indicating copy to clipboard operation
SpacemanDMM copied to clipboard

Feature Request: SHOULD_RESPECT_PROC_SIGNATURE

Open FluffyGhoster opened this issue 1 year ago • 1 comments

Basically what we discussed in Coderbus:

It's possible to create awful and confusing scenarios using the proc inheritance and changing the arguments type of the proc, eg:

atom/proc/mything(mob/A)

atom/movable/mything(mob/living/A) <- this should error out, type is different

area/mything(mob/B) <- this should (optionally) error out, var name is different

turf/mything(mob/A, mob/living/B) <- ok, it's only extending

turf/simulated/mything(mob/A, mob/B) <- this should (optionally) error out, type is different

It would be useful to have a per-proc option to set, which follows eg. this rules:

--- If SHOULD_RESPECT_PROC_SIGNATURE(LAZY): ---

  • All inheritances should use args of the same type or a supertype (mob/living -> mob is ok, the other way it's not)
  • The signature of the proc should either match, be empty or extend with additional arguments after the parent ones are respected

--- If SHOULD_RESPECT_PROC_SIGNATURE(STRICT): ---

  • All of SHOULD_RESPECT_PROC_SIGNATURE(LAZY)
  • All inheritances should use the same type of argument
  • All inheritances should use the same name for the argument (this makes it more bearable to read the proc, than names keeping changing between parent and childs)
  • The signature of the proc should either match or extend with additional arguments the parent one

Optionally, there should be a pragma/setting to enforce it codebase-wide, ontop of the per-proc option (which is more transitional)

FluffyGhoster avatar Sep 16 '24 16:09 FluffyGhoster

+1, I would want this enforced codebase-wise, though I would not allow extending a procs signature with more variables.

/datum/proc/test(mob/a) should fail when /datum/extension/test(mob/a, atom/b) is encountered since it adds space for errors. Or the logic should be way more complex, and it should check to make sure that if you are using test(a, b) then you are calling it on a /datum/extension and not a variable typed with /datum

PowerfulBacon avatar Sep 29 '24 12:09 PowerfulBacon