EncapsulateField is using Set on non-object variables

[daFreeMan]

Excel 2010 32-bit, RD 2.0.11

Starting with this:

Private pLastNPSCol As Long

I execute Refactor | Encapsulate Field, and notice I don't have a choice on Let vs Set

RD ignores the item that is checked (`Let') and I end up with this:

Public Property Get LastNPSCol() As Long
  Set LastNPSCol = pLastNPSCol
End Property

Public Property Let LastNPSCol(ByVal value As Long)
  pLastNPSCol = value
End Property

Which, amazingly, doesn't compile.

[retailcoder]

Huh weird, IMO the refactoring shouldn't even be enabled for a field that's already private...

[retailcoder]

Hmm I'm confusing the inspection and the refactoring.

The command's CanExecute implementation doesn't care for the field's accessibility:

    protected override bool CanExecuteImpl(object parameter)
    {
        var pane = Vbe.ActiveCodePane;
        if (pane == null || _state.Status != ParserState.Ready)
        {
            return false;
        }

        var target = _state.FindSelectedDeclaration(pane);

        var canExecute = target != null 
            && target.DeclarationType == DeclarationType.Variable
            && !target.ParentScopeDeclaration.DeclarationType.HasFlag(DeclarationType.Member);

        return canExecute;
    }

The inspection does:

    public override IEnumerable<InspectionResultBase> GetInspectionResults()
    {
        var issues = UserDeclarations
                        .Where(declaration => declaration.DeclarationType == DeclarationType.Variable
                                            && declaration.Accessibility == Accessibility.Public)
                        .Select(issue => new EncapsulatePublicFieldInspectionResult(this, issue, State, _indenter))
                        .ToList();

        return issues;
    }

So, assuming that's all by design, we're left with a bug in the refactoring. Looking at the GetPropertyText method, the part that creates the Property Get member uses a ternary operator to determine whether to include a Set keyword:

        var getterText = string.Join(Environment.NewLine,
            string.Format(Environment.NewLine + "Public Property Get {0}() As {1}", _model.PropertyName,
                _model.TargetDeclaration.AsTypeName),
            string.Format("    {0}{1} = {2}", !_model.CanImplementLet || _model.ImplementSetSetterType ? "Set " : string.Empty, _model.PropertyName, _model.TargetDeclaration.IdentifierName),
            "End Property" + Environment.NewLine);

So the only way a Set keyword gets added is if !_model.CanImplementLet || _model.ImplementSetSetterType is true, which makes sense. Seems the bug is in the EncapsulateFieldPresenter, which needs a little refactoring since it's defining its own list of primitive types.

If the field is assigned (from anywhere), seems the model has these properties set:

_view.MustImplementLetSetterType = true;
_view.CanImplementSetSetterType = false;

...which seems correct. This one needs investigation.