fix($compile): be more careful about shadowing $attrs values

Shadows only when attributes are non-optional and not own properties,
only stores the observed '@' values when they are indeed strings.

Partial revert of 531a8de

Author: caitp

src/ng/compile.js

@@ -2566,36 +2566,32 @@ function $CompileProvider($provide, $$sanitizeUriProvider) {
         lastValue,
         parentGet, parentSet, compare;
 
-        if (!hasOwnProperty.call(attrs, attrName)) {
-          // In the case of user defined a binding with the same name as a method in Object.prototype but didn't set
-          // the corresponding attribute. We need to make sure subsequent code won't access to the prototype function
-          attrs[attrName] = undefined;
-        }
-
         switch (mode) {
 
           case '@':
-            if (!attrs[attrName] && !optional) {
-              destination[scopeName] = undefined;
+            if (!optional && !hasOwnProperty.call(attrs, attrName)) {
+              destination[scopeName] = attrs[attrName] = void 0;
             }
-
             attrs.$observe(attrName, function(value) {
-              destination[scopeName] = value;
+              if (isString(value)) {
+                destination[scopeName] = value;
+              }
             });
             attrs.$$observers[attrName].$$scope = scope;
-            if (attrs[attrName]) {
+            if (isString(attrs[attrName])) {
               // If the attribute has been provided then we trigger an interpolation to ensure
               // the value is there for use in the link fn
               destination[scopeName] = $interpolate(attrs[attrName])(scope);
             }
             break;
 
           case '=':
-            if (optional && !attrs[attrName]) {
-              return;
+            if (!hasOwnProperty.call(attrs, attrName)) {
+              if (optional) break;
+              attrs[attrName] = void 0;
             }
-            parentGet = $parse(attrs[attrName]);
 
+            parentGet = $parse(attrs[attrName]);
             if (parentGet.literal) {
               compare = equals;
             } else {
@@ -2634,7 +2630,8 @@ function $CompileProvider($provide, $$sanitizeUriProvider) {
             break;
 
           case '&':
-            parentGet = $parse(attrs[attrName]);
+            // Don't assign Object.prototype method to scope
+            parentGet = attrs.hasOwnProperty(attrName) ? $parse(attrs[attrName]) : noop
 
             // Don't assign noop to destination if expression is not valid
             if (parentGet === noop && optional) break;

test/ng/compileSpec.js

@@ -2521,10 +2521,17 @@ describe('$compile', function() {
                 };
 
                 expect(func).not.toThrow();
-                expect(element.find('span').scope()).toBe(element.isolateScope());
-                expect(element.isolateScope()).not.toBe($rootScope);
-                expect(element.isolateScope()['constructor']).toBe($rootScope.constructor);
-                expect(element.isolateScope()['valueOf']).toBeUndefined();
+                var scope = element.isolateScope();
+                expect(element.find('span').scope()).toBe(scope);
+                expect(scope).not.toBe($rootScope);
+
+                // Not shadowed because optional
+                expect(scope.constructor).toBe($rootScope.constructor);
+                expect(scope.hasOwnProperty('constructor')).toBe(false);
+
+                // Shadowed with undefined because not optional
+                expect(scope.valueOf).toBeUndefined();
+                expect(scope.hasOwnProperty('valueOf')).toBe(true);
               })
             );
 
@@ -2539,10 +2546,13 @@ describe('$compile', function() {
                   };
 
                   expect(func).not.toThrow();
-                  expect(element.find('span').scope()).toBe(element.isolateScope());
-                  expect(element.isolateScope()).not.toBe($rootScope);
-                  expect(element.isolateScope()['constructor']).toBe('constructor');
-                  expect(element.isolateScope()['valueOf']).toBe('valueOf');
+                  var scope = element.isolateScope();
+                  expect(element.find('span').scope()).toBe(scope);
+                  expect(scope).not.toBe($rootScope);
+                  expect(scope.constructor).toBe('constructor');
+                  expect(scope.hasOwnProperty('constructor')).toBe(true);
+                  expect(scope.valueOf).toBe('valueOf');
+                  expect(scope.hasOwnProperty('valueOf')).toBe(true);
                 })
             );
 
@@ -2553,10 +2563,17 @@ describe('$compile', function() {
                   };
 
                   expect(func).not.toThrow();
-                  expect(element.find('span').scope()).toBe(element.isolateScope());
-                  expect(element.isolateScope()).not.toBe($rootScope);
-                  expect(element.isolateScope()['constructor']).toBe($rootScope.constructor);
-                  expect(element.isolateScope()['valueOf']).toBeUndefined();
+                  var scope = element.isolateScope();
+                  expect(element.find('span').scope()).toBe(scope);
+                  expect(scope).not.toBe($rootScope);
+
+                  // Does not shadow value because optional
+                  expect(scope.constructor).toBe($rootScope.constructor);
+                  expect(scope.hasOwnProperty('constructor')).toBe(false);
+
+                  // Shadows value because not optional
+                  expect(scope.valueOf).toBeUndefined();
+                  expect(scope.hasOwnProperty('valueOf')).toBe(true);
                 })
             );
 
@@ -4415,6 +4432,34 @@ describe('$compile', function() {
         childScope.theCtrl.test();
       });
     });
+
+    it('should not overwrite @-bound property each digest when not present', function() {
+      module(function($compileProvider) {
+        $compileProvider.directive('testDir', valueFn({
+          scope: {},
+          bindToController: {
+            prop: '@'
+          },
+          controller: function() {
+            var self = this;
+            this.prop = this.prop || 'default';
+            this.getProp = function() {
+             return self.prop;
+            };
+          },
+          controllerAs: 'ctrl',
+          template: '<p></p>'
+        }));
+      });
+      inject(function($compile, $rootScope) {
+        element = $compile('<div test-dir></div>')($rootScope);
+        var scope = element.isolateScope();
+        expect(scope.ctrl.getProp()).toBe('default');
+
+        $rootScope.$digest();
+        expect(scope.ctrl.getProp()).toBe('default');
+      });
+    });
   });