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

Messages and contstructor cannot be conditionally compiled #1231

Closed
athei opened this issue Apr 24, 2022 · 6 comments · Fixed by #1458 or #1707
Closed

Messages and contstructor cannot be conditionally compiled #1231

athei opened this issue Apr 24, 2022 · 6 comments · Fixed by #1458 or #1707
Assignees
Labels
A-ink_lang [ink_lang] Work item C-bug Something isn't working OpenZeppelin

Comments

@athei
Copy link
Contributor

athei commented Apr 24, 2022

Minimal reproducer: https://ink-playground.substrate.io/?id=d666be89f36785e487036bf43d7b175b

It is the same for constructors. I would expect for the message to be just not included in the contract. Instead I get this error:

error[E0599]: the function or associated item `call` exists for struct `lol::Lol`, but its trait bounds were not satisfied
--> lib.rs:18:16
|
8 | pub struct Lol {}
| --------------
| |
| function or associated item `call` not found for this
| doesn't satisfy `lol::Lol: std::ops::Fn<_>`
...
18 | pub fn call(&self) {}
| ^^^^ function or associated item cannot be called on `lol::Lol` due to unsatisfied trait bounds
|
= note: the following trait bounds were not satisfied:
`lol::Lol: std::ops::Fn<_>`
which is required by `&lol::Lol: std::ops::Fn<_>`
note: the following trait must be implemented
= help: items from traits can only be used if the trait is implemented and in scope
= note: the following trait defines an item `call`, perhaps you need to implement it:
candidate #1: `std::ops::Fn`
@SkymanOne
Copy link
Contributor

Trying to open the reproducer link. It is broken. Can you double check whether the issue is still relevant @athei ?

@SkymanOne
Copy link
Contributor

SkymanOne commented Oct 21, 2022

I have looked into this. Here are my findings:

  • Conditionally labeled message functions does not get included after the macro expansion
  • But DispatchableMessageInfo, Label and MessageSpec do get generated for it
  • But it still doesn't get included in the metadata.

So my guess: because we try to generate and use DispatchableMessageInfo and other type definitions for conditionally compiled message but the message function doesn't get included itself, it fails.
I think we need to do some refinement in dispatch.rs to also conditionally generate type definitions for messages

@SkymanOne
Copy link
Contributor

SkymanOne commented Nov 25, 2022

Reopening issue as my solution was not working and #1479 got reverted.

After digging around I have not found an ultimate solution to this problem as the codegen in ir is too cumbersome.

Problem statement

There are certain problems associated with conditionally compilation of contracts elements (messages, constructors, events):

  • The number of spans is counted here before the macro expansion and compilation takes place. It means that even though the function is omitted during compilation stage, its span will be counted during codegen
  • The list of message ids is also generated before macro expansion here. Same problems occurs here too, we generate an id for an element, include it into intermediate representation before it gets omitted at a compilation stage.

The above list of examples is not exhaustive and there are others places where similar pre-compilation evaluation occur. That means that we can not just extract cfg attributes and place them above codegen blocks for specific element.

Possible solutions

It is actually hard to call them solutions. More like workaround.

  1. Conditionally evaluate consts of elements. The problem occur when the generated const block refers to a function that was omitted during compilation stage here. We can potentially wrap this expression inside cfg! or cfg-if! macro and if the condition is met, assign a placeholder function.

This solution is not ideal because there still be traces of omitted function.

  1. Filter out elements using cfg! macro based on features. Exactly same approach as in reverted PR, but manually extract feature name and put it inside macro:
...
if cfg!(feature = "<feature_name>") {
   ...
}
...

Pretty cumbersome solution which doesn't account for other conditional compilation flags and require syntactical analysis which make it brittle.

  1. Break down codegen into 2 stages. The first stage will clean-up the original codebase and evaluate all the conditional compilation attributes and macros. The output code will be pipelined into the second stage the ir is actually generated.
    Downsides: requires too much effort for a relatively insignificant issue. It can have more use cases in future, but I can't think of them right now. Secondly, compilation times will increase as we now need to evaluate the same code block twice.

@SkymanOne SkymanOne added the A-ink_lang [ink_lang] Work item label Nov 25, 2022
@cmichi cmichi moved this to Todo in OpenZeppelin ↔ ink! Mar 2, 2023
@SkymanOne SkymanOne moved this from Todo to In Progress in OpenZeppelin ↔ ink! Mar 5, 2023
@SkymanOne
Copy link
Contributor

After the digging around, I came into the conclusion that we need to change the way we generate intermediate representation for messages and constructors.

As an example I will refer to DispatchableMessageInfo<const ID: u32>

Currently, everything single message "artefact" in ir is indexed with uniquely ID. This ID is stored in const array IDS. In order to pull the relevant types and bounds related to the message, its ID is retrieved from the IDS array like here
This creates problem when we want to omit some of the messages. Because the passed indexes are based on message spans that are counted (like here before the macro expansion and omission, we have out-of-bound problems.

My proposal is to remove constant array IDS from the codegen and explicitly pass generated IDs in the codegen.

@SkymanOne SkymanOne mentioned this issue Mar 7, 2023
8 tasks
@cmichi
Copy link
Collaborator

cmichi commented Mar 9, 2023

Why does the message even land in the IR in the first place? Can't we just ignore it during parsing?

PS: I've updated the minimal reproducer in the issue description to a working link.

@SkymanOne
Copy link
Contributor

SkymanOne commented Mar 9, 2023

Why does the message even land in the IR in the first place? Can't we just ignore it during parsing?

PS: I've updated the minimal reproducer in the issue description to a working link.

No, we can't, cfg attribute is expanded at the same time as macros. So we don't know whether the conditional compilation condition is satisfied or not until the compile stage.

My PR provides a solution to this. It will completely omit any presence of the message in IR.

@github-project-automation github-project-automation bot moved this from In Progress to Done in OpenZeppelin ↔ ink! Mar 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-ink_lang [ink_lang] Work item C-bug Something isn't working OpenZeppelin
Projects
None yet
3 participants