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

Allow methods to inspect warnings attached to self, if requested #6222

Open
radeusgd opened this issue Apr 6, 2023 · 9 comments
Open

Allow methods to inspect warnings attached to self, if requested #6222

radeusgd opened this issue Apr 6, 2023 · 9 comments
Assignees
Labels
-compiler -libs Libraries: New libraries to be implemented l-writedata p-low Low priority x-new-feature Type: new feature request x-on-hold

Comments

@radeusgd
Copy link
Member

radeusgd commented Apr 6, 2023

As noted in #6176:

we realised we may actually need this behaviour [the ability to inspect warnings attached to self] to be more generalized and work not only with the Warning type. Actually, for the Write capabilities, we want by default to avoid writing if the value contains any warnings. To be able to effectively stop a table.write some args operation if table has warnings attached, we will need Table.write to also have an ability to see any warnings attached to self (and due to how CB is designed, we want to keep this dispatched as table.write some args and not Some_Writer_Type.write table some args).

We should have some way to mark any method to be able to have special handling of attached warnings.

The idea is following:

type My_Type
    Value x

    method_1 self =
        IO.println (Warning.get_all self)

    @some_way_to_tell_that_we_want_warnings_preserved_here
    method_2 self =
        IO.println (Warning.get_all self)

main =
    a = My_Type.Value 42
    b = Warning.attach "WARN" a
    b.method_1 # prints []
    b.method_2 # prints a non-empty vector containing the warning

We want to keep the current behaviour for now, so that b.method_1 would still print [] and the warnings are passed on to the result, but from inside of the invocation they are not seen.

But for specially marked functions (be it with an @annotation or some other special marking (maybe we could reuse the ~ sign which has no sensible meaning on self currently? just an idea)) we want to be able to 'see' all warnings that were attached to a value from where it was being invoked. So in b.method_2 we'd expect it to print the warning WARN.

This will allow us to implement stuff like #6176 in pure Enso without special handling. More importantly, it will allow us to implement the warning checks needed for the Write operations.

@4e6 4e6 added x-new-feature Type: new feature request -libs Libraries: New libraries to be implemented labels Apr 7, 2023
@4e6 4e6 removed the triage label Apr 7, 2023
@JaroslavTulach
Copy link
Member

I believe @hubertp is already working on such a functionality. See:

@jdunkerley jdunkerley added this to the Beta Release milestone Apr 11, 2023
@radeusgd
Copy link
Member Author

I believe @hubertp is already working on such a functionality. See:

This issue is a follow-up to #6027, as noted in a comment in the PR attached to it.

@hubertp
Copy link
Contributor

hubertp commented Apr 11, 2023

Having a custom annotation to indicate that we should leave the warnings alone was one idea that I proposed when attempting to fix the original PR/issue. It's unfortunately much more involving because a lot of places assume that warnings do not wrap self during function invocation. So that could have some non-trivial implications.

A better solution would be to support warnings natively, something that @JaroslavTulach started with a draft PR in GraalVM. This would be a more stable and performance-oriented solution. It will also take longer to get in that change to the official release.

@radeusgd
Copy link
Member Author

A better solution would be to support warnings natively, something that @JaroslavTulach started with a draft PR in GraalVM. This would be a more stable and performance-oriented solution.

Cool! Is there a link to the draft PR so one can follow its progress?

It will also take longer to get in that change to the official release.

This may be an issue, since I think we will need to be able to provide the ability to 'avoid writing if there are warnings attached' relatively soon - but that is a venue that @jdunkerley will know best I suppose.

@JaroslavTulach
Copy link
Member

JaroslavTulach commented Apr 11, 2023

Please note that "invoking methods on self with warnings" is basically doing the same thing as https://github.com/orgs/enso-org/discussions/6216 - e.g. the invoking a method with completely different self.

GitHub
The context The PR #3764 introduced a calling convention that allows to write Foo.method (Mk_Foo 123) as a synonym of (Mk_Foo 123).method (just to note, the status of the static/instance calls befo...

@radeusgd
Copy link
Member Author

Please note that "invoking methods on self with warnings" is basically doing the same thing as github.com/orgs/enso-org/discussions/6216 - e.g. the invoking a method with completely different self.

Well, I'd say this applies to the current PR #6027 - as indeed it delegates each call of x.has_warnings to Warning.has_warnings x for any value x that has warnings attached.

That's exactly why I don't like that solution and I'm advocating for another one in this task.

With this task, we wouldn't be modifying the dispatch mechanism - every invocation of x.has_warnings will be resolved as usual. So usually, this will mean Any.has_warnings is called, since every of our objects is a 'subtype' of Any. Then, Any.has_warnings self would be specially annotated so that it can access the warnings it has on the self. So no special magic is happening to the dispatch mechanism - it is only dictated by the type of x. That's why I prefer this solution.

So yeah, this is another argument for this task :) (although the main argument is still that we need it to implement the 'do not run write if the value has warnings attached, unless overridden' logic - to which I don't think we can have any other workaround that is sane)

@Akirathan
Copy link
Member

Related to #6070.

@radeusgd
Copy link
Member Author

Related to #6070.

@Akirathan Is it? I think that #6070 is about builtin methods and this issue is mostly aimed at methods written in Enso, so these seem to be separate things.

@Akirathan
Copy link
Member

@Akirathan Is it? I think that #6070 is about builtin methods and this issue is mostly aimed at methods written in Enso, so these seem to be separate things.

Oh, right, sorry - yes, #6070 is only about stripping warnings from self for only builtin methods.

@jdunkerley jdunkerley removed this from the Beta Release milestone Jul 18, 2023
@jdunkerley jdunkerley added the p-low Low priority label Sep 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
-compiler -libs Libraries: New libraries to be implemented l-writedata p-low Low priority x-new-feature Type: new feature request x-on-hold
Projects
Status: 📤 Backlog
Development

No branches or pull requests

6 participants