use-ink / ink

Polkadot's ink! to write smart contracts.
https://use.ink
Apache License 2.0
1.36k stars 431 forks source link

Messages and contstructor cannot be conditionally compiled #1231

Closed athei closed 1 year ago

athei commented 2 years ago

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 commented 2 years ago

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

SkymanOne commented 2 years ago

I have looked into this. Here are my findings:

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 commented 1 year ago

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 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.

  2. 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 commented 1 year ago

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.

cmichi commented 1 year ago

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 commented 1 year ago

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.