ApeWorX / ape-vyper

Vyper compiler plugin for the Ape Framework, using VVM
https://www.apeworx.io/
Apache License 2.0
26 stars 9 forks source link

Compiler errors not displayed when there is a compilation issue #26

Closed fubuloubu closed 2 years ago

fubuloubu commented 2 years ago

I was debugging what the cause behind this issue was:

$ ape compile                                                                                                                                      
INFO: Compiling 'contracts/ApePiece.vy'.                                                                                                                                                   
ERROR: (VyperInstallError) Unable to install Vyper version: 0.3.1.

It turned out to be a legitimate bug! However, due to how this code works, it's not displaying the bug properly, just showing something about the it not being able to install the version of Vyper it's currently using: https://github.com/ApeWorX/ape-vyper/blob/c6f9e50b4ecfe6c1f7e8eb78501391a53b688dee/ape_vyper/compiler.py#L128-L133

Was thinking why it was necessary to catch this exception? Is it just so err inherits from CompilerError? Can this be done another way so it any compiler errors look transparent to the user?

This is the actual error that should be displayed (which I access when I debugged):

$ ape compile                                                                                                                                      
INFO: Compiling 'contracts/ApePiece.vy'.                                                                                                                                                   
> .../site-packages/ape_vyper/compiler.py(140)compile()                                                                           
-> new_err = VyperInstallError(vyper_version)
(Pdb) print(err.stderr_data)
vyper.exceptions.StateAccessViolation: not allowed to query contract or environment variables in pure functions
  contract "/tmp/vyper-6dd2eazp.vy", function "_tokenIdToString", line 132:8 
       131     return concat(
  ---> 132         self._digitToString(digit1),
  -----------------^
       133         self._digitToString(digit2),

[20400] Failed to execute script 'vyper_compile' due to unhandled exception!

Related: would be nice to use vvm.compile_files so we get the actual source filename in there.

fubuloubu commented 2 years ago

A little better:

class VyperCompileError(CompilerError):
    def __init__(self, err):
        if hasattr(err, "stderr_data"):
            message = err.stderr_data
        else:
            message = str(err)

        self.message = message
        super().__init__(message)

...
class VyperCompiler(CompilerAPI):
    ...
    def compile(self, contract_filepaths: List[Path]) -> List[ContractType]:
        ...
            try:                                 
                result = vvm.compile_source(                                                
                    source,                                                   
                    vyper_version=vyper_version,
                )["<stdin>"]
            except Exception as err:
                raise VyperCompileError(err) from err
$ ape compile
INFO: Compiling 'contracts/ApeToken.sol'.
INFO: Compiling 'contracts/ApePiece.vy'.
ERROR: (VyperCompileError) vyper.exceptions.StateAccessViolation: not allowed to query contract or environment variables in pure functions
  contract "/tmp/vyper-6dd2eazp.vy", function "_tokenIdToString", line 132:8 
       131     return concat(
  ---> 132         self._digitToString(digit1),
  -----------------^
       133         self._digitToString(digit2),

[20400] Failed to execute script 'vyper_compile' due to unhandled exception!
antazoey commented 2 years ago

Was thinking why it was necessary to catch this exception? Is it just so err inherits from CompilerError?

Yeah, it was raising Abort() before and then we upgraded. Raising a custom error like CompilerError is like the same thing as raise Abort() -- to get displayed to the user is a nice way. That is the only change I made on this so far. Upgrades!

The install-error does not seem related to the compile errors though, or is it? I am little confused there.

Yeah, I agree that exceptions in general can be improved for this plugin.


Edit, oh I see we are raising a Intall Error when we are supposed to be raising a Compile Error.

fubuloubu commented 2 years ago

Edit, oh I see we are raising a Intall Error when we are supposed to be raising a Compile Error.

Yes lol