From 203e63d9383ffb9260d91fa4aa20828497b38b7a Mon Sep 17 00:00:00 2001 From: antazoey Date: Tue, 10 Sep 2024 16:36:35 -0500 Subject: [PATCH] fix: mistakenly changed sources to in-memory manifest (#2272) --- src/ape/managers/project.py | 62 ++++++++++++++------------- src/ape_pm/compiler.py | 10 +---- tests/functional/test_project.py | 26 +++++++++-- tests/integration/cli/test_compile.py | 27 ++++++++++++ 4 files changed, 84 insertions(+), 41 deletions(-) diff --git a/src/ape/managers/project.py b/src/ape/managers/project.py index ae62a12b96..dd9f1f5971 100644 --- a/src/ape/managers/project.py +++ b/src/ape/managers/project.py @@ -2125,34 +2125,37 @@ def __getattr__(self, item: str) -> Any: message = getattr(err, "message", str(err)) did_append = False - all_files = get_all_files_in_directory(self.contracts_folder) - for path in all_files: - # Possibly, the user was trying to use a file name instead. - if path.stem != item: - continue + if item not in (self.manifest.contract_types or {}): + all_files = get_all_files_in_directory(self.contracts_folder) + for path in all_files: + # Possibly, the user was trying to use a file name instead. + if path.stem != item: + continue - if message and message[-1] not in (".", "?", "!"): - message = f"{message}." - message = ( - f"{message} However, there is a source file named '{path.name}', " - "did you mean to reference a contract name from this source file?" - ) - did_append = True - break - - # Possibly, the user does not have compiler plugins installed or working. - missing_exts = set() - for path in all_files: - if ext := get_full_extension(path): - if ext not in self.compiler_manager.registered_compilers: - missing_exts.add(ext) - - if missing_exts: - start = "Else, could" if did_append else "Could" - message = ( - f"{message} {start} it be from one of the " - "missing compilers for extensions: " + f'{", ".join(sorted(missing_exts))}?' - ) + if message and message[-1] not in (".", "?", "!"): + message = f"{message}." + + message = ( + f"{message} However, there is a source file named '{path.name}'. " + "This file may not be compiling (see error above), or maybe you meant " + "to reference a contract name from this source file?" + ) + did_append = True + break + + # Possibly, the user does not have compiler plugins installed or working. + missing_exts = set() + for path in all_files: + if ext := get_full_extension(path): + if ext not in self.compiler_manager.registered_compilers: + missing_exts.add(ext) + + if missing_exts: + start = "Else, could" if did_append else "Could" + message = ( + f"{message} {start} it be from one of the " + "missing compilers for extensions: " + f'{", ".join(sorted(missing_exts))}?' + ) err.args = (message,) raise # The same exception (keep the stack the same height). @@ -2356,9 +2359,10 @@ def isolate_in_tempdir(self, **config_override) -> Iterator["LocalProject"]: yield self else: - # Add sources to manifest memory, in case they are missing. - self._manifest.sources = dict(self.sources.items()) + sources = dict(self.sources.items()) with super().isolate_in_tempdir(**config_override) as project: + # Add sources to manifest memory, in case they are missing. + project.manifest.sources = sources yield project def unpack(self, destination: Path, config_override: Optional[dict] = None) -> "LocalProject": diff --git a/src/ape_pm/compiler.py b/src/ape_pm/compiler.py index 01f75b8185..297af2ad25 100644 --- a/src/ape_pm/compiler.py +++ b/src/ape_pm/compiler.py @@ -54,15 +54,7 @@ def compile( code = src_path.read_text() source_id = source_ids[path] - try: - # NOTE: Always set the source ID to the source of the JSON file - # to avoid manifest corruptions later on. - contract_type = self.compile_code(code, project=project, sourceId=source_id) - except CompilerError as err: - logger.warning( - f"Unable to parse {ContractType.__name__} from '{source_id}'. Reason: {err}" - ) - continue + contract_type = self.compile_code(code, project=project, sourceId=source_id) # NOTE: Try getting name/ ID from code-JSON first. # That's why this is not part of `**kwargs` in `compile_code()`. diff --git a/tests/functional/test_project.py b/tests/functional/test_project.py index 9528f147bb..9da19395b2 100644 --- a/tests/functional/test_project.py +++ b/tests/functional/test_project.py @@ -148,6 +148,25 @@ def test_isolate_in_tempdir(project): assert not tmp_project.manifest_path.is_file() +def test_isolate_in_tempdir_does_not_alter_sources(project): + # First, create a bad source. + with project.temp_config(contracts_folder="tests"): + new_src = project.contracts_folder / "newsource.json" + new_src.write_text("this is not json, oops") + project.sources.refresh() # Only need to be called when run with other tests. + + try: + with project.isolate_in_tempdir() as tmp_project: + # The new (bad) source should be in the temp project. + assert "tests/newsource.json" in (tmp_project.manifest.sources or {}), project.path + finally: + new_src.unlink() + project.sources.refresh() + + # Ensure "newsource" did not persist in the in-memory manifest. + assert "tests/newsource.json" not in (project.manifest.sources or {}) + + def test_in_tempdir(project, tmp_project): assert not project.in_tempdir assert tmp_project.in_tempdir @@ -200,8 +219,9 @@ def test_getattr_same_name_as_source_file(project_with_source_files_contract): expected = ( r"'LocalProject' object has no attribute 'ContractA'\. " r"Also checked extra\(s\) 'contracts'\. " - r"However, there is a source file named 'ContractA\.sol', " - r"did you mean to reference a contract name from this source file\? " + r"However, there is a source file named 'ContractA\.sol'\. " + r"This file may not be compiling \(see error above\), " + r"or maybe you meant to reference a contract name from this source file\? " r"Else, could it be from one of the missing compilers for extensions: .*" ) with pytest.raises(AttributeError, match=expected): @@ -587,7 +607,7 @@ def test_project_api_foundry_and_ape_config_found(foundry_toml): def test_get_contract(project_with_contracts): actual = project_with_contracts.get_contract("Other") - assert isinstance(actual, ContractContainer) + assert isinstance(actual, ContractContainer), f"{type(actual)}" assert actual.contract_type.name == "Other" # Ensure manifest is only loaded once by none-ing out the path. diff --git a/tests/integration/cli/test_compile.py b/tests/integration/cli/test_compile.py index 0020b074fa..a551f8fad2 100644 --- a/tests/integration/cli/test_compile.py +++ b/tests/integration/cli/test_compile.py @@ -147,6 +147,33 @@ def test_compile_when_sources_change(ape_cli, runner, integ_project, clean_cache assert "contracts/Interface.json" not in result.output +@skip_projects_except("multiple-interfaces") +def test_compile_when_sources_change_problematically(ape_cli, runner, integ_project, clean_cache): + """ + There was a bug when sources changes but had errors, that the old sources continued + to be used and the errors were swallowed. + """ + source_path = integ_project.contracts_folder / "Interface.json" + content = source_path.read_text() + assert "bar" in content, "Test setup failed - unexpected content" + + result = runner.invoke( + ape_cli, ("compile", "--project", f"{integ_project.path}"), catch_exceptions=False + ) + assert result.exit_code == 0, result.output + + # Change the contents of a file in a problematic way. + source_path = integ_project.contracts_folder / "Interface.json" + modified_source_text = source_path.read_text().replace("{", "BRACKET") + source_path.unlink() + source_path.touch() + source_path.write_text(modified_source_text, encoding="utf8") + result = runner.invoke( + ape_cli, ("compile", "--project", f"{integ_project.path}"), catch_exceptions=False + ) + assert result.exit_code != 0, result.output + + @skip_projects_except("multiple-interfaces") def test_compile_when_contract_type_collision(ape_cli, runner, integ_project, clean_cache): source_path = integ_project.contracts_folder / "Interface.json"