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

.forbids pragma: defining forbidden tags #20050

Merged
merged 21 commits into from
Jul 26, 2022
Merged

.forbids pragma: defining forbidden tags #20050

merged 21 commits into from
Jul 26, 2022

Conversation

Lancer11211
Copy link
Contributor

@Lancer11211 Lancer11211 commented Jul 17, 2022

Hi,
I've made this to handle my use case we've discussed under this forum post. My tests are located at tests/effects/teffects1(1-7).nim. Let me know if you're willing to accept this PR and if anything needs to be done before merging. Thank you!

@Araq
Copy link
Member

Araq commented Jul 17, 2022

Excellent work. But we need to bikeshed about the name. "notTags"? How about "prevents"? "avoids"? "forbids"?

@Lancer11211
Copy link
Contributor Author

Thank you! forbids sounds good to me. Let me know if it's final and I'll update the code.

@Lancer11211
Copy link
Contributor Author

Meanwhile, I've added another test: tests/effects/teffects12.nim - this is the one I mentioned in the forum post. It's a simple code which enables safe locking for a resource while disabling recurrent locks.

This patch intends to define the opposite of the .tags pragma: a way to define effects which are not allowed in a proc.
@Lancer11211
Copy link
Contributor Author

I've renamed the pragma to forbids and truncated the commits.

@Lancer11211 Lancer11211 changed the title .notTags pragma: defining forbidden tags .forbids pragma: defining forbidden tags Jul 17, 2022
@Araq
Copy link
Member

Araq commented Jul 17, 2022

We also need a manual update, a changelog entry and an RFC. Maybe the RFC got already written though. And I don't say this to demotivate you, I'll help you push it through.

We need to focus especially on the subtyping relation between type T = proc () {.forbids: [E].} and type T = proc () < no forbids specified>.

@Lancer11211
Copy link
Contributor Author

Of course, I'll update the doc/manual.md and the changelog.md. I'll let you know when it's ready. I think this RFC is relevant: nim-lang/RFCs#302

Lancer added 3 commits July 17, 2022 17:24
the forbids pragma didn't handle simple restrictions properly and it also had issues with subtyping
@Lancer11211
Copy link
Contributor Author

Lancer11211 commented Jul 17, 2022

While I was writing the documentation, I've found 2 bugs in the patch so I fixed them(and added 2 tests as coverage) before pushing this documentation/changelog update - let me know if you need anything added.

@Araq
Copy link
Member

Araq commented Jul 17, 2022

The type relation must be documented and justified.

@Lancer11211
Copy link
Contributor Author

Do you mean like this?

@arnetheduck
Copy link
Contributor

arnetheduck commented Jul 18, 2022

Excellent work. But we need to bikeshed about the name. "notTags"? How about "prevents"? "avoids"? "forbids"?

At the core of this need for a new pragma lies a semantic difference: tags are additive while raises are not, ergo tags needs two pragmas - if we treat tags like exceptions (more or less), the alternative design here is that we have a pragma that works like raises which explicitly lists all tags that a function can propagate instead of "adding" and "forbidding".

wrong, per commend below

doc/manual.md Show resolved Hide resolved
@Araq
Copy link
Member

Araq commented Jul 18, 2022

tags are additive while raises are not

I don't know why you keep phrasing it this way. Raises are additive too, except that try except can undo a raises effect. A general "undo tag" feature can be currently approximated via {.cast(tags: []).}. With the possible extension .cast(onlyAllows: [E])

@arnetheduck
Copy link
Contributor

I don't know why you keep phrasing it this way.

my bad, got confused by some test code

@Lancer11211
Copy link
Contributor Author

I've corrected the patch to properly support proc type subtyping and I've updated the manual too.

doc/manual.md Outdated
caller2(toBeCalled2)

`ProcType2` is a subtype of `ProcType1`. Unlike with tags, the parent context
doesn't inherit the forbidden list of effects.
Copy link
Member

Choose a reason for hiding this comment

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

What does that mean "the parent context doesn't inherit"? What is the "parent context"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let's say function1 calls function2 - then function1 is the "parent context". I'm calling it a context because it's a context of effects in terms of the effect system. I can't really articulate it, but I wanted to refer to a hierarchy of function calls and statements.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've added a little clarification to that line.

@Araq
Copy link
Member

Araq commented Jul 20, 2022

The CI is red because you need to update the docgen tests too.

Please run:

nim c -r -d:nimTestsNimdocFixup nimdoc/tester.nim

@Lancer11211
Copy link
Contributor Author

Thanks! I updated the docs and pull from devel.

@Lancer11211
Copy link
Contributor Author

It seems like I'm still missing something:

FAILURE: files differ: nimdoc/testproject/htmldocs/testproject.html
diff --git a/nimdoc/testproject/expected/testproject.html b/nimdoc/testproject/htmldocs/testproject.html

I tried nim c -r -d:nimTestsNimdocFixup nimdoc/testproject/testproject.nim but it didn't change anything.

@Lancer11211
Copy link
Contributor Author

Seems like the CI passed. Previously, I used nim c -r ... instead of ./bin/nim c -r ... which messed up the generated files.

@Lancer11211
Copy link
Contributor Author

Do we need anything else for the merge?

changelog.md Outdated Show resolved Hide resolved
compiler/semcall.nim Outdated Show resolved Hide resolved
@Araq Araq merged commit efd5c57 into nim-lang:devel Jul 26, 2022
@github-actions
Copy link
Contributor

Thanks for your hard work on this PR!
The lines below are statistics of the Nim compiler built from efd5c57

Hint: mm: orc; threads: on; opt: speed; options: -d:release
163520 lines; 15.201s; 841.215MiB peakmem

@Lancer11211
Copy link
Contributor Author

Thanks for the review and the merge!

@dom96
Copy link
Contributor

dom96 commented Jul 26, 2022

This is beautiful, thank you for implementing it!

FedericoCeratto pushed a commit to FedericoCeratto/Nim that referenced this pull request Jul 30, 2022
* .forbids pragma: defining illegal effects for proc types

This patch intends to define the opposite of the .tags pragma: a way to define effects which are not allowed in a proc.

* updated documentation and changelogs for the forbids pragma

* renamed notTagEffects to forbiddenEffects

* corrected issues of forbids pragma

the forbids pragma didn't handle simple restrictions properly and it also had issues with subtyping

* removed incorrect character from changelog

* added test to cover the interaction of methods and the forbids pragma

* covering the interaction of the tags and forbids pragmas

* updated manual about the forbids pragma

* removed useless statement

* corrected the subtyping of proc types using the forbids pragma

* updated manual for the forbids pragma

* updated documentations for forbids pragma

* updated nim docs

* updated docs with rsttester.nim

* regenerated documentation

* updated rst docs

* Update changelog.md

Co-authored-by: ringabout <[email protected]>

* updated changelog

* corrected typo

Co-authored-by: ringabout <[email protected]>
capocasa pushed a commit to capocasa/Nim that referenced this pull request Mar 31, 2023
* .forbids pragma: defining illegal effects for proc types

This patch intends to define the opposite of the .tags pragma: a way to define effects which are not allowed in a proc.

* updated documentation and changelogs for the forbids pragma

* renamed notTagEffects to forbiddenEffects

* corrected issues of forbids pragma

the forbids pragma didn't handle simple restrictions properly and it also had issues with subtyping

* removed incorrect character from changelog

* added test to cover the interaction of methods and the forbids pragma

* covering the interaction of the tags and forbids pragmas

* updated manual about the forbids pragma

* removed useless statement

* corrected the subtyping of proc types using the forbids pragma

* updated manual for the forbids pragma

* updated documentations for forbids pragma

* updated nim docs

* updated docs with rsttester.nim

* regenerated documentation

* updated rst docs

* Update changelog.md

Co-authored-by: ringabout <[email protected]>

* updated changelog

* corrected typo

Co-authored-by: ringabout <[email protected]>
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.

6 participants