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

detect unconsumed/unsent messages, introduce generated dummy messages #15

Merged
merged 5 commits into from
Dec 9, 2022

Conversation

drahnr
Copy link
Collaborator

@drahnr drahnr commented Dec 8, 2022

If a message type is never sent or never received, this leads to From<$ty> implementations being missing.

The implementation here adds a check and throws and error pointint to the messages.

Closes #13

@drahnr drahnr requested a review from sandreim December 8, 2022 08:40
@drahnr
Copy link
Collaborator Author

drahnr commented Dec 8, 2022

Note, currently one integration test errors due to exactly that fact:

error: Message MsgStrukt is sent but never received
  --> orchestra/examples/solo.rs:28:40
   |
28 |     #[subsystem(consumes: Plinko, sends: [MsgStrukt])]
   |                                           ^^^^^^^^^

error: Message Plinko is received but never sent
  --> orchestra/examples/solo.rs:28:24
   |
28 |     #[subsystem(consumes: Plinko, sends: [MsgStrukt])]
   |                           ^^^^^^

error: could not compile `orchestra` due to 2 previous errors

@drahnr drahnr force-pushed the bernhard-better-error-messages branch from 06b1f43 to c4a164e Compare December 8, 2022 08:43
@sandreim
Copy link
Collaborator

sandreim commented Dec 8, 2022

LGTM, but I think we should add a test for this extra check and fix the failing one.

@drahnr
Copy link
Collaborator Author

drahnr commented Dec 8, 2022

Just tried with polkadot, and it needs to also address the case where a subsystem only actually sends messages (i.e. BitfieldSigningSubsystem, PvfChecker). Currently this would error out.

I am thinking of allowing an _ for the receiving part, such that the two dummy / non instantiable enums can be deleted

@drahnr
Copy link
Collaborator Author

drahnr commented Dec 8, 2022

So the impl details are as follows:

  • create an internal dummy message type, if none is specified
  • the generated type is not taken into account when doing the graph or sanity checks, and hence the checks will pass
  • the channels are still created in a fully connected fashion, just as before, due to some bigger refactoring required to avoid them, which - again - points to Avoid dependency on consumed message type, generate a marker type #11

To use this in polkadot, delete the two unused messages paritytech/polkadot#6413

@drahnr drahnr force-pushed the bernhard-better-error-messages branch from e1c222b to ac6db47 Compare December 8, 2022 18:11
@drahnr drahnr changed the title add an error message if some messages are lopsided detect unconsumed/unsent messages, introduce generated dummy messages Dec 8, 2022
@drahnr drahnr requested a review from vstakhov December 9, 2022 14:29
@drahnr
Copy link
Collaborator Author

drahnr commented Dec 9, 2022

I'll sign all commits, one sec, I missed that before.

If a message type is never sent or never received,
this leads to From<$ty> implementations being missing.

The implementation here adds a check and throws and error
pointint to the messages.

Closes #13

Signed-off-by: Bernhard Schuster <bernhard@ahoi.io>
Signed-off-by: Bernhard Schuster <bernhard@ahoi.io>
Signed-off-by: Bernhard Schuster <bernhard@ahoi.io>
They need to publicly accessible after all, for the Context and
Subsystem trait bounds.

Signed-off-by: Bernhard Schuster <bernhard@ahoi.io>
Signed-off-by: Bernhard Schuster <bernhard@ahoi.io>
@drahnr drahnr force-pushed the bernhard-better-error-messages branch from da5c278 to 8e5adf1 Compare December 9, 2022 15:28
@drahnr drahnr merged commit f2d1840 into master Dec 9, 2022
drahnr added a commit that referenced this pull request Dec 13, 2022
…#15)

Error out if a message is not sent that is consumed and vice versa

If a message type is never sent or never received,
this leads to From<$ty> implementations being missing.

The implementation here adds a check and throws and error
pointing to the messages in question.

It also allows to skip the consuming message without a placeholder
and generated a dummy message of the form "{$ty}Message" which
have to be imported for those subsystems.

Closes #13

Signed-off-by: Bernhard Schuster <bernhard@ahoi.io>
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.

Confusing error for unconsumed message
2 participants