Skip to content

Repro and Fix for issue #2529#2531

Merged
crusso merged 7 commits intomasterfrom
claudio/issue-2529
May 17, 2021
Merged

Repro and Fix for issue #2529#2531
crusso merged 7 commits intomasterfrom
claudio/issue-2529

Conversation

@crusso
Copy link
Contributor

@crusso crusso commented May 15, 2021

No description provided.

@crusso crusso changed the title Repro for issue 2529 Repro for issue #2529 May 15, 2021
@crusso crusso linked an issue May 15, 2021 that may be closed by this pull request
@dfinity-ci
Copy link

This PR does not affect the produced WebAssembly code.

@crusso
Copy link
Contributor Author

crusso commented May 17, 2021

@nomeata PTAL moved the repro to test/mo-idl, it's proper home

@crusso crusso requested a review from chenyan-dfinity May 17, 2021 12:34
@crusso
Copy link
Contributor Author

crusso commented May 17, 2021

So the bug is that the Stamp map, used to distinguish instantiation of the same constructor for monomorphisation, was being shared between constructors with the same name but different stamp (and possibly completely different defs). I've now added another outer level of indexing (__k_n) using k to distinguish cons with the same name but different stamp. I suppress k and n when either is 1.

Which raises another question, why do we number form 1, not 0, so the first renaming is always _2 (not _1)? Was there a rationale for this?

In another branch, I'll experiment with normalizing the type to avoid all these weird indirections.

@crusso crusso changed the title Repro for issue #2529 Repro and Fix for issue #2529 May 17, 2021
Copy link
Contributor

@chenyan-dfinity chenyan-dfinity left a comment

Choose a reason for hiding this comment

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

Thanks for fix!

Which raises another question, why do we number form 1, not 0, so the first renaming is always _2 (not _1)? Was there a rationale for this?

Yeah, it's an arbitrary choice, 0 is probably better.

@crusso
Copy link
Contributor Author

crusso commented May 17, 2021

Thanks for fix!

Which raises another question, why do we number form 1, not 0, so the first renaming is always _2 (not _1)? Was there a rationale for this?

Yeah, it's an arbitrary choice, 0 is probably better.

Ok, I'll renumber and also normalize defs.

@crusso
Copy link
Contributor Author

crusso commented May 17, 2021

@chenyan-dfinity PTAL

Copy link
Contributor

@chenyan-dfinity chenyan-dfinity left a comment

Choose a reason for hiding this comment

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

LGTM

@crusso crusso linked an issue May 17, 2021 that may be closed by this pull request
@crusso crusso merged commit 3c6577a into master May 17, 2021
@crusso crusso deleted the claudio/issue-2529 branch May 17, 2021 20:41
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.

Type renamings yield convoluted idl [Bug] Type renaming(s) incorrectly rejected as cyclic

4 participants

Comments