DeadCodeDetector: Report unused fields in contract address types #1164
Conversation
jubnzv
force-pushed
the
1099_dcd_arg_fields
branch
from
bf81e3a to
221bf97
Compare
There was a problem hiding this comment.
A few minor comments, which are really just nitpicking.
Does this work for nested address types as well? I.e, something like this:
transition (x : ByStr20 with contract
field y : ByStr20 with contract
field used : Int32,
field unused : Uint32
end
end)
a <-& x.y;
b <-& y.used;
end
src/base/DeadCodeDetector.ml
Outdated
| let check_contract_address_types_in_params used_contract_params | ||
| contract_params comp = | ||
| let address_params = | ||
| List.fold_left comp.comp_params ~init:contract_params | ||
| ~f:(fun m (id, ty) -> update_contract_params_map m id ty) | ||
| in | ||
| let used_addresses = | ||
| (* addr |-> set of used fields *) | ||
| List.fold_left comp.comp_body ~init:emp_idsmap ~f:(fun m s -> | ||
| get_used_address_fields address_params s |> merge_id_maps m) | ||
| in | ||
| (* Generate warnings for unused fields in component arguments. *) | ||
| List.iter comp.comp_params ~f:(fun (id, ty) -> | ||
| let name = get_id id in | ||
| match (Map.find address_params name, Map.find used_addresses name) with | ||
| | Some fields, Some used_fields | ||
| when not @@ phys_equal (Set.length fields) (Set.length used_fields) | ||
| -> ( | ||
| match ty with | ||
| | SType.Address (ContrAddr m) -> | ||
| List.iter (SType.IdLoc_Comp.Map.keys m) ~f:(fun id -> | ||
| let name = get_id id in | ||
| if Set.mem fields name && (not @@ Set.mem used_fields name) | ||
| then | ||
| warn1 | ||
| ("Unused field in the contract address type: " | ||
| ^ as_error_string id) | ||
| warning_level_dead_code | ||
| (SType.TIdentifier.get_rep id)) | ||
| | _ -> ()) | ||
| | _ -> ()); | ||
| (* Collect fields of the [cmod] contract with contract address types used | ||
| in this component. *) | ||
| Map.keys used_addresses | ||
| |> List.fold_left ~init:used_contract_params ~f:(fun used_ids_map used_id -> | ||
| match Map.find used_ids_map used_id with | ||
| | Some used_fields -> | ||
| let data = | ||
| Map.find_exn used_addresses used_id |> Set.union used_fields | ||
| in | ||
| Map.set used_ids_map ~key:used_id ~data | ||
| | None -> | ||
| let data = Map.find_exn used_addresses used_id in | ||
| Map.set used_ids_map ~key:used_id ~data) | ||
|
|
||
| (** Checks for unused fields in contract address types occurred in contract | ||
| parameters and parameters of contract's components. *) | ||
| let check_param_contract_address_types cmod = | ||
| let contract_params = | ||
| List.fold_left cmod.contr.cparams ~init:emp_idsmap ~f:(fun m (id, ty) -> | ||
| update_contract_params_map m id ty) | ||
| in | ||
| let used_contract_params = | ||
| List.fold_left cmod.contr.ccomps ~init:emp_idsmap ~f:(fun used c -> | ||
| check_contract_address_types_in_params used contract_params c) | ||
| in | ||
| (* Report unused contract parameters with contract address types. *) | ||
| Map.keys contract_params | ||
| |> List.iter ~f:(fun param -> | ||
| match Map.find used_contract_params param with | ||
| | Some used_fields -> | ||
| let all_fields = Map.find_exn contract_params param in | ||
| let unused_fields = Set.diff all_fields used_fields in | ||
| if not @@ Set.is_empty unused_fields then | ||
| List.iter cmod.contr.cparams ~f:(fun (id, _ty) -> | ||
| if SCIdentifier.Name.equal param (SCIdentifier.get_id id) | ||
| then | ||
| Set.iter unused_fields ~f:(fun f -> | ||
| warn1 | ||
| ("Unused field in the contract address type: " | ||
| ^ SCIdentifier.Name.as_string f) | ||
| warning_level_dead_code | ||
| (ER.get_loc (SCIdentifier.get_rep id)))) | ||
| | None -> | ||
| (* All the fields of [param] are unused, so we don't report it. | ||
| It is an unused contract parameter.*) | ||
| ()) |
There was a problem hiding this comment.
These functions look very similar. Is there a way to avoid the code duplication here, or is there a subtle difference between the functions that I'm missing?
There was a problem hiding this comment.
Sorry, which ones do you mean?
We need a different logic to handle contract parameters, component parameters and ADT fields.
There was a problem hiding this comment.
The functions are check_contract_address_types_in_params and check_param_contract_address_types.
Looking at the code again I can see what you mean, but I'm surprised the logic is that different. Anyway, not a big deal.
There was a problem hiding this comment.
Well, I created small functions to reuse similar logic used in both functions.
Yes, it has the similar patterns like these folds, but I think, it will be less readable if we extract functions from them.
| "warning_message": "Unused procedure: use_all", | ||
| "start_location": { | ||
| "file": "contracts/dead_code_test17.scilla", | ||
| "line": 17, | ||
| "column": 11 | ||
| }, | ||
| "end_location": { "file": "", "line": 0, "column": 0 }, | ||
| "warning_id": 3 | ||
| }, | ||
| { | ||
| "warning_message": "Unused contract parameter: all_used", | ||
| "start_location": { | ||
| "file": "contracts/dead_code_test17.scilla", | ||
| "line": 10, | ||
| "column": 5 | ||
| }, | ||
| "end_location": { "file": "", "line": 0, "column": 0 }, | ||
| "warning_id": 3 | ||
| }, |
There was a problem hiding this comment.
I was about to complain that all_used is actually used, but then I realised that you are much more clever than I am. :-)
Good job.
It could be implemented, but is it worth it? I think these cases are too rare to care about them, but it will complicate the checker. |
I'm not sure how relevant it is. It sort of feels like a loose thread that we ought to tie up, but it might be better to ship what we have, and deal with nested address types later. I do think we should open an issue on it, though, so that we don't forget. |
|
I created the following issues about recursive fields: #1182 |
|
Looks good. |
jubnzv
force-pushed
the
1099_dcd_arg_fields
branch
from
023648b to
47b2761
Compare
To avoid potential crashes on refactoring
Closes #1099