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

Jump to bottom

Can not use argument-definition-tests in inner function. #8007

Closed
DartBot opened this issue Jan 21, 2013 · 8 comments
Closed

Can not use argument-definition-tests in inner function. #8007

DartBot opened this issue Jan 21, 2013 · 8 comments
Assignees
@crelier
Labels
area-vm Use area-vm for VM related issues, including code coverage, FFI, and the AOT and JIT backends.
Milestone
M3

Comments

@DartBot
Copy link

DartBot commented Jan 21, 2013

This issue was originally filed by @tomochikahara

What steps will reproduce the problem?

  1. Declare this function below.

List<Date> toDate(List<String> dateStrings, [onError(String)]) {
  var df = new DateFormat("yyyyMMdd");
  return dateStrings.map((str) {
    try {
      return df.parse(str);
    } catch(e) {
      if (?onError) {
        onError(str);
      } else {
        throw e;
      }
    }
  });
}

  1. Call this function.

What is the expected output? What do you see instead?

  I expected an argument-definition-test be evaluated correctly.
  But, I got an error below.

  file:///..()../sample.dart': Error: line 11 pos 12: formal parameter name expected
  if (?onError) {

What version of the product are you using? On what operating system?

  Dart Editor version 0.2.10_r16761
  Dart SDK version 0.2.10.1_r16761

  OS : Max OS X 10.8.2

Please provide any additional information below.

  I guess that an argument-definition-test just refers to parameters of the most inner function.
  This restriction doesn't exist in Dart's Language Specification.
@sethladd
Copy link
Contributor

Double checking with Gilad if this is possible.

In the meantime, you can check if onError is null, yes?

cc @gbracha.
Added Area-Language, Triaged labels.

@DartBot
Copy link
Author

DartBot commented Jan 22, 2013

This comment was originally written by @tomochikahara

I already have worked around it by assignment to a temporary variable.
But I think yours is more expressing to my intent.

Thanks a lot for your advice, and I hope the issue would be fix.

@gbracha
Copy link
Contributor

gbracha commented Jan 22, 2013

It should work. This is an implementation bug, not a language issue. Recategorizing accordingly.

Removed Area-Language label.
Added Area-VM label.

@kmillikin
Copy link

Yes, that's a bug in the VM. It can see why it doesn't work, but I'm not sure what's the right way to fix it.

The code in LocalScope::RestoreOuterScope builds a malformed scope chain, in the sense that there are variables that can be found by looping up the scope chain but it is impossible to find their owning scope by looping up the scope chain.

The code in Parser::IsFormalParameter assumes that the scope chain is well-formed in the sense above.

It seems reasonable to always have such well-formed scope chains, but the current code in LocalScope::RestoreOuterScope seems intentional so I'm not sure what will break if we change it. Regis?

cc @crelier.

@kmillikin
Copy link

A secondary issue is that variables apparently do not themselves know that they are parameters, except implicitly by appearing at a position k st. 0 <= k < argc in their owning scope.

Scopes deserialized by LocalScope::RestoreOuterScope do not have this property.

@iposva-google
Copy link
Contributor

Set owner to @crelier.
Added this to the M3 milestone.

@crelier
Copy link
Contributor

crelier commented Jan 24, 2013

Thanks for the report, test case, and analysis.
I will fix it tomorrow Thursday.

Added Accepted label.

@crelier
Copy link
Contributor

crelier commented Jan 24, 2013

The issue here was that the parameter was from an outer scope and was also captured. Being captured, it was found by the lookup and therefore the code walking the scope chain was used. As Kevin indicated, the scope chain is not setup for the outer scope.
The fix is to recognize that the parameter is from the outer scope and use the same code as in the case were the parameter is not captured.

Fixed at r17592.

cc @kmillikin.
Added Fixed label.

@DartBot DartBot added Type-Defect area-vm Use area-vm for VM related issues, including code coverage, FFI, and the AOT and JIT backends. labels Jan 24, 2013
@DartBot DartBot added this to the M3 milestone Jan 24, 2013
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@crelier crelier

Labels
area-vm Use area-vm for VM related issues, including code coverage, FFI, and the AOT and JIT backends.
Projects
None yet
Milestone
M3
Development

No branches or pull requests
6 participants
@sethladd @kmillikin @gbracha @iposva-google @crelier @DartBot