Skip to content

lower top-level statements so that the front-end knows their values are unused #26304

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

Merged
merged 1 commit into from
Mar 4, 2018

Conversation

JeffBezanson
Copy link
Member

Fixes most false positives in the deprecation for using the value of .=; see #26088 (comment)

This could also be useful in other ways in the future.

@JeffBezanson JeffBezanson added compiler:lowering Syntax lowering (compiler front end, 2nd stage) deprecation This change introduces or involves a deprecation labels Mar 2, 2018
@StefanKarpinski
Copy link
Member

Does it make sense to test that module M; x .= y; nothing; end doesn't warn or error?

@JeffBezanson
Copy link
Member Author

JeffBezanson commented Mar 3, 2018

Ok, I'm adding tests for this. It was bizarrely difficult. First I found that @test_warn and @test_nowarn simply don't work for this; don't know why. @test_deprecated doesn't have any way to test that something is not deprecated (it seems to, since you can pass a pattern, but passing a pattern that matches the empty string doesn't work). Finally I figured out that, apparently, @test_logs f(x) tests that evaluating f(x) does not log anything. Obviously!

their values are not used

fixes most false positives in the deprecation for using the value of `.=`
@StefanKarpinski
Copy link
Member

@c42f: is the @test_logs behavior intentional? Should it be called @test_no_log instead?

@fredrikekre
Copy link
Member

The docstring for @test_logs is very good, and have examples for how to test the logs

help?> @test_logs
  @test_logs [log_patterns...] [keywords] expression

  Collect a list of log records generated by expression using collect_test_logs, check that they match the sequence log_patterns, and return the value of expression. The
  keywords provide some simple filtering of log records: the min_level keyword controls the minimum log level which will be collected for the test, the match_mode keyword
  defines how matching will be performed (the default :all checks that all logs and patterns match pairwise; use :any to check that the pattern matches at least once somewhere
  in the sequence.)

  The most useful log pattern is a simple tuple of the form (level,message). A different number of tuple elements may be used to match other log metadata, corresponding to the
  arguments to passed to AbstractLogger via the handle_message function: (level,message,module,group,id,file,line). Elements which are present will be matched pairwise with the
  log record fields using == by default, with the special cases that Symbols may be used for the standard log levels, and Regexs in the pattern will match string or Symbol
  fields using contains.

  Examples
  ≡≡≡≡≡≡≡≡≡≡

  Consider a function which logs a warning, and several debug messages:

  function foo(n)
      @info "Doing foo with n=$n"
      for i=1:n
          @debug "Iteration $i"
      end
      42
  end

  We can test the info message using

  @test_logs (:info,"Doing foo with n=2") foo(2)

  If we also wanted to test the debug messages, these need to be enabled with the min_level keyword:

  @test_logs (:info,"Doing foo with n=2") (:debug,"Iteration 1") (:debug,"Iteration 2") min_level=Debug foo(2)

  The macro may be chained with @test to also test the returned value:

  @test (@test_logs (:info,"Doing foo with n=2") foo(2)) == 42

@JeffBezanson
Copy link
Member Author

The docstring is very good; the surprising part is that you pass zero patterns to check for zero log events. We could add a @test_nolog convenience wrapper for that case.

@JeffBezanson JeffBezanson merged commit f569ebe into master Mar 4, 2018
@JeffBezanson JeffBezanson deleted the jb/unusedvalues branch March 4, 2018 15:47
mbauman added a commit that referenced this pull request Mar 5, 2018
…luenonscalarindexedassignment

* origin/master: (28 commits)
  fix an optimizer bug in `invoke` with non-constant functions (#26301)
  lower top-level statements in such a way that the front-end knows (#26304)
  Make sure Sockets page has h1 header (#26315)
  fix doctests, and make them less prone to errors (#26275)
  FIx intro to manual chapter on types (#26312)
  Add a missing "that" (#26313)
  fix docstring for code_llvm (#26266)
  Remove the examples/ folder (#26153)
  download cert.pem from deps-getall, fixes #24903 (#25344)
  Slight update to doc string for at-enum to refer to instances (#26208)
  performance tweak in reverse(::String) (#26300)
  remove references to `TCPSocket` in Base docstrings (#26298)
  Deprecate adding integers to CartesianIndex (#26284)
  Deprecate conj(::Any), add real(::Missing) and imag(::Missing) (#26288)
  fix #26267, regression in `names` with `imported=false` (#26293)
  fix #25857, `typeinfo` problem in showing arrays with abstract tuple types (#26289)
  Add adjoint(::Missing) method (#25502)
  Use lowered form in logging macro (#26291)
  deprecate bin, oct, dec, hex, and base in favor of `string` and keyword args (#25804)
  deprecate `spawn(cmd)` to `run(cmd, wait=false)` (#26130)
  ...
@mlechu mlechu mentioned this pull request Apr 16, 2025
topolarity added a commit that referenced this pull request Apr 23, 2025
This is preparatory work for experimental integration with
JuliaLowering, which
happened to be separable into its own PR.

We currently have three lowering entry points doing slightly different
things:
`jl-expand-to-thunk`, `jl-expand-to-thunk-warn` (for complaining about
ambiguous
soft scope assignments during non-interactive use), and
`jl-expand-to-thunk-stmt` (which wraps the expression in a block
returning
nothing before lowering it) and a bunch of C wrappers on top of those
(see red
nodes in the call graphs below).

In this PR:
- Make all lowering calls go through `jl_lower`, which just calls out to
lisp
for now, but will probably function like `jl_parse` does in a future PR.
  - Handle `warn` with an extra parameter
- Delete most of the lowering wrappers, which were mainly a result of
the
    three entry points
- While we're breaking things in ast.c, take the opportunity to rename
  "expand"-prefixed functions to "lower"-prefixed ones (excluding macro
  expansion functions, which are called as the first part of lowering).

Here's a call graph before this change, made by looking for callers
of each lowering entry point and tracing back one or more steps (expect
mistakes...). Macro expansion functions are mostly omitted for clarity.
Blue is scheme, red is ast.c, and green is toplevel.c.


![graph](https://github.com/user-attachments/assets/b84a8662-06b4-45f6-937f-2d5ab5325ff0)

After this change:

![graph(3)](https://github.com/user-attachments/assets/dd3765e2-1cd4-4ab8-bf85-6fa6eb7f9a74)


#### todo?
- ~~I'd like to see if we can eliminate the `stmt` boolean option from
`jl_lower`
and handle that another way; I'm just not sure it's worth the effort at
the
moment. We only use it in one place in `jl_eval_module_expr`. The
original
  code path was added in #26304.~~
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler:lowering Syntax lowering (compiler front end, 2nd stage) deprecation This change introduces or involves a deprecation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants