Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore: only warn about KeyError "ast"/"metadata" if debugging #331

Closed
wants to merge 3 commits into from

Conversation

karmacoma-eth
Copy link
Collaborator

@karmacoma-eth karmacoma-eth commented Jul 25, 2024

plus a drive-by simplification of Mapper

@karmacoma-eth karmacoma-eth requested a review from daejunpark July 25, 2024 00:07
Comment on lines 1373 to 1378
except KeyError as err:
if str(err) in ["ast", "metadata"]:
# somewhat expected, e.g. for sol files containing only libraries
if args.debug:
debug(
f"Skipped {json_filename} due to parsing failure: "
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is the real change, the rest is a refactoring

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what's your intention when err is not ast nor metadata? generate warning, or silently ignore?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

another q: would it be possible to check if the ast / metadata missing error is indeed due to libraries?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

another q: would it be possible to check if the ast / metadata missing error is indeed due to libraries?

that was the case every time I looked

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what's your intention when err is not ast nor metadata? generate warning, or silently ignore?

oops, the intention is to generate a warning for other errors

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what's your intention when err is not ast nor metadata? generate warning, or silently ignore?

oops, the intention is to generate a warning for other errors

in that case, i don't think the current code works as intended.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed in b135251

Comment on lines -1382 to -1383
except Exception:
pass
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is what prompted the refactoring, we don't want to ignore all exceptions

@@ -1300,8 +1296,24 @@ def mk_arrlen(args: HalmosConfig) -> Dict[str, int]:
return arrlen


def parse_symbols(args: HalmosConfig, contract_map, contract_name):
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pulling this in its own method to avoid nested try..except blocks

def parse_build_out(args: HalmosConfig) -> Dict:
result = {} # compiler version -> source filename -> contract name -> (json, type)
# compiler version -> source filename -> contract name -> (json, type)
result = defaultdict(lambda: defaultdict(dict))
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

opportunistic, this avoids doing if x not in y: y[x] = {} a bunch of times

Comment on lines +1333 to +1340
if not json_filename.endswith(".json"):
continue
if json_filename.startswith("."):
continue

json_path = os.path.join(sol_path, json_filename)
with open(json_path, encoding="utf8") as f:
json_out = json.load(f)
json_path = os.path.join(sol_path, json_filename)
with open(json_path, encoding="utf8") as f:
json_out = json.load(f)
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pulling this out of the try..except block to reduce the scope of the try blocks and minimize nesting

@karmacoma-eth
Copy link
Collaborator Author

should address #281

@daejunpark
Copy link
Collaborator

daejunpark commented Jul 26, 2024

could you separate the main change from the refactoring changes, to make it easier to review the core change? could be a separate pr, or just a separate commit.

@daejunpark
Copy link
Collaborator

[starting a new thread for easier discussion]

another q: would it be possible to check if the ast / metadata missing error is indeed due to libraries?

that was the case every time I looked

ah, my question was, whether it's possible to automatically check if the problematic file is a library or not, and generate the error message only when it is not a library.

indeed, i'm concerned about silently ignoring field-missing errors in non-debug mode. although we haven't seen cases where non-library files are skipped due to missing fields, i believe we still need to allow users review the skipped files, unless we're certain they can be safely skipped.

also, the usability issue described in #281 is not about the warning message itself, but rather that the message is not detailed enough to understand what's happening, and its tone is too strong.

so, my suggestion for the short term is to improve the warning message, allowing users to review them and detect any real issues, without being scared.

a longer-term solution is to implement a mechanism to check whether the given file is a library and ignore the error only when it's a library.

@karmacoma-eth
Copy link
Collaborator Author

closing this one, it's getting too messy. Will update the investigation in #281

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants