From a95373f303e62a4cc8be8b080c50cbd4e2ce268f Mon Sep 17 00:00:00 2001 From: Contrabang <91113370+Contrabang@users.noreply.github.com> Date: Fri, 19 Apr 2024 14:30:13 -0400 Subject: [PATCH] Adds CI to restrict types in a file (#24694) * LETS GAMBLE! * oops * even more oops * alright lets fuckin do it * real names please * comment * Contributing.md update * maybe someone will find this todo * actually lets make it a define so people can't mistype it * review * changes --- .github/CONTRIBUTING.md | 21 +++++- .github/workflows/ci.yml | 1 + code/__DEFINES/antag_defines.dm | 7 ++ code/__DEFINES/misc_defines.dm | 3 + code/_onclick/hud/hud_datum.dm | 4 ++ .../antagonists/_common/antag_datum.dm | 2 + .../changeling/datum_changeling.dm | 5 +- .../modules/antagonists/cult/datum_cultist.dm | 2 + code/modules/antagonists/cult/team_cult.dm | 2 + .../revolutionary/datum_headrev.dm | 1 + .../revolutionary/datum_revolutionary.dm | 2 + .../revolutionary/team_revolution.dm | 2 + .../antagonists/traitor/datum_mindslave.dm | 5 +- .../antagonists/traitor/datum_traitor.dm | 1 + .../modules/antagonists/vampire/vamp_datum.dm | 27 +------ .../antagonists/vampire/vamp_thrall.dm | 22 ++++++ paradise.dme | 1 + tools/ci/restrict_file_types.py | 71 +++++++++++++++++++ 18 files changed, 147 insertions(+), 32 deletions(-) create mode 100644 code/modules/antagonists/vampire/vamp_thrall.dm create mode 100644 tools/ci/restrict_file_types.py diff --git a/.github/CONTRIBUTING.md b/.github/CONTRIBUTING.md index e711d1a72069..c3eb80df962f 100644 --- a/.github/CONTRIBUTING.md +++ b/.github/CONTRIBUTING.md @@ -586,6 +586,26 @@ SS13 has a lot of legacy code that's never been updated. Here are some examples - Files and path accessed and referenced by code above simply being #included should be strictly lowercase to avoid issues on filesystems where case matters. +#### Modular Code in a File + +Code should be modular where possible; if you are working on a new addition, then strongly consider putting it in its own file unless it makes sense to put it with similar ones (i.e. a new tool would go in the `tools.dm` file) + +Our codebase also has support for checking files so that they only contain one specific typepath, including none of its subtypes. This can be done by adding a specific header at the beginning of the file, which the CI will look for when running. An example can be seen below. You can also run this test locally using `/tools/ci/restrict_file_types.py` + +```dm +RESTRICT_TYPE(/datum/foo) + +/datum/proc/do_thing() // Error: '/datum' proc found in a file restricted to '/datum/foo' + +/datum/foo + +/datum/foo/do_thing() + +/datum/foo/bar // Error: '/datum/foo/bar' type definition found in a file restricted to '/datum/foo' + +/datum/foo/bar/do_thing() // Error: '/datum/foo/bar' proc found in a file restricted to '/datum/foo' +``` + ### SQL - Do not use the shorthand sql insert format (where no column names are specified) because it unnecessarily breaks all queries on minor column changes and prevents using these tables for tracking outside related info such as in a connected site/forum. @@ -679,7 +699,6 @@ SS13 has a lot of legacy code that's never been updated. Here are some examples ### Other Notes -- Code should be modular where possible; if you are working on a new addition, then strongly consider putting it in its own file unless it makes sense to put it with similar ones (i.e. a new tool would go in the `tools.dm` file) - Bloated code may be necessary to add a certain feature, which means there has to be a judgement over whether the feature is worth having or not. You can help make this decision easier by making sure your code is modular. - You are expected to help maintain the code that you add, meaning that if there is a problem then you are likely to be approached in order to fix any issues, runtimes, or bugs. diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 1c5f9e4eb6f8..25df92cd3481 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -48,6 +48,7 @@ jobs: python tools/ci/unticked_files.py ${GITHUB_WORKSPACE} python tools/ci/illegal_dme_files.py ${GITHUB_WORKSPACE} python tools/ci/define_sanity.py + python tools/ci/restrict_file_types.py python -m tools.ci.check_icon_conflicts python -m tools.ci.check_icon_dupenames python -m tools.maplint.source --github diff --git a/code/__DEFINES/antag_defines.dm b/code/__DEFINES/antag_defines.dm index 7b88e983ac2f..fe136abd200e 100644 --- a/code/__DEFINES/antag_defines.dm +++ b/code/__DEFINES/antag_defines.dm @@ -61,3 +61,10 @@ GLOBAL_LIST(contractors) * Pulse Demon */ #define PULSEDEMON_SOURCE_DRAIN_INVALID (-1) + +/proc/ischangeling(mob/M) // TODO: Someone please convert these to proper defines some day. + return M.mind?.has_antag_datum(/datum/antagonist/changeling) + +// Helper proc that determines if a mob is a mindslave. +/proc/ismindslave(mob/living/carbon/human/H) + return istype(H) && H.mind.has_antag_datum(/datum/antagonist/mindslave, FALSE) diff --git a/code/__DEFINES/misc_defines.dm b/code/__DEFINES/misc_defines.dm index 2a35068d3cd2..c1ab48c1f79f 100644 --- a/code/__DEFINES/misc_defines.dm +++ b/code/__DEFINES/misc_defines.dm @@ -695,3 +695,6 @@ do { \ #define TEAM_ADMIN_ADD_OBJ_SUCCESS (1<<0) #define TEAM_ADMIN_ADD_OBJ_CANCEL_LOG (1<<1) #define TEAM_ADMIN_ADD_OBJ_PURPOSEFUL_CANCEL (1<<2) + +/// A helper used by `restrict_file_types.py` to identify types to restrict in a file. Not used by byond at all. +#define RESTRICT_TYPE(type) // do nothing diff --git a/code/_onclick/hud/hud_datum.dm b/code/_onclick/hud/hud_datum.dm index 6ac4cce04b8e..804e0c0f90f7 100644 --- a/code/_onclick/hud/hud_datum.dm +++ b/code/_onclick/hud/hud_datum.dm @@ -252,3 +252,7 @@ /datum/hud/proc/update_locked_slots() return + +/datum/hud/proc/remove_vampire_hud() + static_inventory -= vampire_blood_display + QDEL_NULL(vampire_blood_display) diff --git a/code/modules/antagonists/_common/antag_datum.dm b/code/modules/antagonists/_common/antag_datum.dm index 653295489883..28c6d0ce9ec7 100644 --- a/code/modules/antagonists/_common/antag_datum.dm +++ b/code/modules/antagonists/_common/antag_datum.dm @@ -1,3 +1,5 @@ +RESTRICT_TYPE(/datum/antagonist) + GLOBAL_LIST_EMPTY(antagonists) #define SUCCESSFUL_DETACH "dont touch this string numbnuts" diff --git a/code/modules/antagonists/changeling/datum_changeling.dm b/code/modules/antagonists/changeling/datum_changeling.dm index edf430caee9f..5822374b983f 100644 --- a/code/modules/antagonists/changeling/datum_changeling.dm +++ b/code/modules/antagonists/changeling/datum_changeling.dm @@ -1,3 +1,5 @@ +RESTRICT_TYPE(/datum/antagonist/changeling) + /datum/antagonist/changeling name = "Changeling" roundend_category = "changelings" @@ -440,8 +442,5 @@ else to_chat(L, "While our current form may be lifeless, this is not the end for us as we can still regenerate!") -/proc/ischangeling(mob/M) - return M.mind?.has_antag_datum(/datum/antagonist/changeling) - /datum/antagonist/changeling/custom_blurb() return "We awaken on the [station_name()], [get_area_name(owner.current, TRUE)]...\nWe have our tasks to attend to..." diff --git a/code/modules/antagonists/cult/datum_cultist.dm b/code/modules/antagonists/cult/datum_cultist.dm index 41f3e898ea00..105b66256bea 100644 --- a/code/modules/antagonists/cult/datum_cultist.dm +++ b/code/modules/antagonists/cult/datum_cultist.dm @@ -1,3 +1,5 @@ +RESTRICT_TYPE(/datum/antagonist/cultist) + /datum/antagonist/cultist name = "Cultist" job_rank = ROLE_CULTIST diff --git a/code/modules/antagonists/cult/team_cult.dm b/code/modules/antagonists/cult/team_cult.dm index fcc26a00dc02..f6c31962fee1 100644 --- a/code/modules/antagonists/cult/team_cult.dm +++ b/code/modules/antagonists/cult/team_cult.dm @@ -1,3 +1,5 @@ +RESTRICT_TYPE(/datum/team/cult) + /datum/team/cult name = "Cult" antag_datum_type = /datum/antagonist/cultist diff --git a/code/modules/antagonists/revolutionary/datum_headrev.dm b/code/modules/antagonists/revolutionary/datum_headrev.dm index 5f98e11c7b44..1f229266badf 100644 --- a/code/modules/antagonists/revolutionary/datum_headrev.dm +++ b/code/modules/antagonists/revolutionary/datum_headrev.dm @@ -1,3 +1,4 @@ +RESTRICT_TYPE(/datum/antagonist/rev/head) /datum/antagonist/rev/head name = "Head Revolutionary" diff --git a/code/modules/antagonists/revolutionary/datum_revolutionary.dm b/code/modules/antagonists/revolutionary/datum_revolutionary.dm index 9f9ffd95e0f3..920d56483fda 100644 --- a/code/modules/antagonists/revolutionary/datum_revolutionary.dm +++ b/code/modules/antagonists/revolutionary/datum_revolutionary.dm @@ -1,3 +1,5 @@ +RESTRICT_TYPE(/datum/antagonist/rev) + /datum/antagonist/rev name = "Revolutionary" roundend_category = "revs" diff --git a/code/modules/antagonists/revolutionary/team_revolution.dm b/code/modules/antagonists/revolutionary/team_revolution.dm index 87a9bb3deb1e..989654a598b1 100644 --- a/code/modules/antagonists/revolutionary/team_revolution.dm +++ b/code/modules/antagonists/revolutionary/team_revolution.dm @@ -1,3 +1,5 @@ +RESTRICT_TYPE(/datum/team/revolution) + /datum/team/revolution name = "Revolution" antag_datum_type = /datum/antagonist/rev diff --git a/code/modules/antagonists/traitor/datum_mindslave.dm b/code/modules/antagonists/traitor/datum_mindslave.dm index e511df1ad0bd..fde537bd8afd 100644 --- a/code/modules/antagonists/traitor/datum_mindslave.dm +++ b/code/modules/antagonists/traitor/datum_mindslave.dm @@ -1,3 +1,4 @@ +RESTRICT_TYPE(/datum/antagonist/mindslave) // For Mindslaves and Zealots /datum/antagonist/mindslave @@ -87,7 +88,3 @@ slaved.serv -= owner slaved.leave_serv_hud(owner) owner.som = null - -// Helper proc that determines if a mob is a mindslave. -/proc/ismindslave(mob/living/carbon/human/H) - return istype(H) && H.mind.has_antag_datum(/datum/antagonist/mindslave, FALSE) diff --git a/code/modules/antagonists/traitor/datum_traitor.dm b/code/modules/antagonists/traitor/datum_traitor.dm index a33f14f52dd4..367054b0616d 100644 --- a/code/modules/antagonists/traitor/datum_traitor.dm +++ b/code/modules/antagonists/traitor/datum_traitor.dm @@ -1,3 +1,4 @@ +RESTRICT_TYPE(/datum/antagonist/traitor) // Syndicate Traitors. /datum/antagonist/traitor diff --git a/code/modules/antagonists/vampire/vamp_datum.dm b/code/modules/antagonists/vampire/vamp_datum.dm index 7ad89d6ef865..251c758c6dc4 100644 --- a/code/modules/antagonists/vampire/vamp_datum.dm +++ b/code/modules/antagonists/vampire/vamp_datum.dm @@ -1,3 +1,5 @@ +RESTRICT_TYPE(/datum/antagonist/vampire) + /datum/antagonist/vampire name = "Vampire" antag_hud_type = ANTAG_HUD_VAMPIRE @@ -33,27 +35,6 @@ blurb_b = 138 blurb_a = 1 -/datum/antagonist/mindslave/thrall - name = "Vampire Thrall" - antag_hud_type = ANTAG_HUD_VAMPIRE - antag_hud_name = "vampthrall" - -/datum/antagonist/mindslave/thrall/add_owner_to_gamemode() - SSticker.mode.vampire_enthralled += owner - -/datum/antagonist/mindslave/thrall/remove_owner_from_gamemode() - SSticker.mode.vampire_enthralled -= owner - -/datum/antagonist/mindslave/thrall/apply_innate_effects(mob/living/mob_override) - mob_override = ..() - var/datum/mind/M = mob_override.mind - M.AddSpell(new /datum/spell/vampire/thrall_commune) - -/datum/antagonist/mindslave/thrall/remove_innate_effects(mob/living/mob_override) - mob_override = ..() - var/datum/mind/M = mob_override.mind - M.RemoveSpell(/datum/spell/vampire/thrall_commune) - /datum/antagonist/vampire/Destroy(force, ...) owner.current.create_log(CONVERSION_LOG, "De-vampired") draining = null @@ -361,9 +342,7 @@ mob_override.dna?.species.hunger_icon = 'icons/mob/screen_hunger_vampire.dmi' check_vampire_upgrade(FALSE) -/datum/hud/proc/remove_vampire_hud() - static_inventory -= vampire_blood_display - QDEL_NULL(vampire_blood_display) + /datum/antagonist/vampire/custom_blurb() return "On the date [GLOB.current_date_string], at [station_time_timestamp()],\n in the [station_name()], [get_area_name(owner.current, TRUE)]...\nThe hunt begins again..." diff --git a/code/modules/antagonists/vampire/vamp_thrall.dm b/code/modules/antagonists/vampire/vamp_thrall.dm new file mode 100644 index 000000000000..112e95173325 --- /dev/null +++ b/code/modules/antagonists/vampire/vamp_thrall.dm @@ -0,0 +1,22 @@ +RESTRICT_TYPE(/datum/antagonist/mindslave/thrall) + +/datum/antagonist/mindslave/thrall + name = "Vampire Thrall" + antag_hud_type = ANTAG_HUD_VAMPIRE + antag_hud_name = "vampthrall" + +/datum/antagonist/mindslave/thrall/add_owner_to_gamemode() + SSticker.mode.vampire_enthralled += owner + +/datum/antagonist/mindslave/thrall/remove_owner_from_gamemode() + SSticker.mode.vampire_enthralled -= owner + +/datum/antagonist/mindslave/thrall/apply_innate_effects(mob/living/mob_override) + mob_override = ..() + var/datum/mind/M = mob_override.mind + M.AddSpell(new /datum/spell/vampire/thrall_commune) + +/datum/antagonist/mindslave/thrall/remove_innate_effects(mob/living/mob_override) + mob_override = ..() + var/datum/mind/M = mob_override.mind + M.RemoveSpell(/datum/spell/vampire/thrall_commune) diff --git a/paradise.dme b/paradise.dme index e45f422f3cbb..922db58d828f 100644 --- a/paradise.dme +++ b/paradise.dme @@ -1469,6 +1469,7 @@ #include "code\modules\antagonists\traitor\contractor\items\contractor_uplink.dm" #include "code\modules\antagonists\traitor\contractor\items\extraction_items.dm" #include "code\modules\antagonists\vampire\vamp_datum.dm" +#include "code\modules\antagonists\vampire\vamp_thrall.dm" #include "code\modules\antagonists\vampire\vampire_subclasses.dm" #include "code\modules\antagonists\vampire\vampire_powers\dantalion_powers.dm" #include "code\modules\antagonists\vampire\vampire_powers\gargantua_powers.dm" diff --git a/tools/ci/restrict_file_types.py b/tools/ci/restrict_file_types.py new file mode 100644 index 000000000000..6185f129fcf2 --- /dev/null +++ b/tools/ci/restrict_file_types.py @@ -0,0 +1,71 @@ +import glob +import os +import re +import sys +import time +from collections import namedtuple + +Failure = namedtuple("Failure", ["filename", "lineno", "message"]) + +RED = "\033[0;31m" +GREEN = "\033[0;32m" +BLUE = "\033[0;34m" +NC = "\033[0m" # No Color + +def print_error(message: str, filename: str, line_number: int): + if os.getenv("GITHUB_ACTIONS") == "true": # We're on github, output in a special format. + print(f"::error file={filename},line={line_number},title=Restricted Type in File::{filename}:{line_number}: {RED}{message}{NC}") + else: + print(f"{filename}:{line_number}: {RED}{message}{NC}") + +if __name__ == "__main__": + print("restrict_file_types started") + + exit_code = 0 + start = time.time() + + dm_files = glob.glob("**/*.dm", recursive=True) + + if len(sys.argv) > 1: + dm_files = sys.argv[1:] + + all_failures = [] + + for code_filepath in dm_files: + with open(code_filepath, encoding="UTF-8") as code: + filename = code_filepath.split(os.path.sep)[-1] + + restrict_regex = re.match(r"RESTRICT_TYPE\((.+)\)", code.read()) + if(not restrict_regex): + continue + + restrict_type_path = restrict_regex.group(1) + + # Matches a definition into two groups: + # 1: The typepath or /proc (for global procs), required. First character is handled specially, to avoid picking up start-of-line comments. + # 2: The name of the proc, if any. + definition_matcher = re.compile(r'^(/[\w][\w/]*?)(?:/proc)?(?:/([\w]+)\(.*?)?(?: */[/*].*)?$') + code.seek(0) + for idx, line in enumerate(code): + if(rematch_result := re.search(definition_matcher, line)): + if(restrict_type_path != rematch_result.group(1)): + type_path = rematch_result.group(1) + proc_name = rematch_result.group(2) + if(type_path == "/proc"): + all_failures += [Failure(code_filepath, idx + 1, f"'Global proc '/proc/{proc_name}' found in a file restricted to type '{restrict_type_path}'")] + else: + if(proc_name): + all_failures += [Failure(code_filepath, idx + 1, f"'Proc '{type_path}/proc/{proc_name}' found in a file restricted to type '{restrict_type_path}'")] + else: + all_failures += [Failure(code_filepath, idx + 1, f"'Definition for different type '{type_path}' found in a file restricted to '{restrict_type_path}'")] + + + if all_failures: + exit_code = 1 + for failure in all_failures: + print_error(failure.message, failure.filename, failure.lineno) + + end = time.time() + print(f"restrict_file_types tests completed in {end - start:.2f}s\n") + + sys.exit(exit_code)